Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/0123 complex funs #1774

Merged
merged 76 commits into from
Mar 28, 2020
Merged

Feature/0123 complex funs #1774

merged 76 commits into from
Mar 28, 2020

Conversation

bob-carpenter
Copy link
Contributor

Summary

Adds support for forward and reverse mode autodiff for all functions in the std::complex spec. The base class implementations were added in a previous PR.

Several of the overloads which may seem redundant are there because of differences between g++ and clang++.

Tests

Extensively unit tested.

Boundary conditions are largely not tested because they're inconsistent between std::complex<T> implementations in g++ and clang++ default libraries.

Side Effects

More overloads of functions like pow and exp and atanh.

Checklist

  • Math issue complex numbers with vars #123 [this completes complex numbers with vars #123, but there's still one more PR coming that adds Eigen matrix support for complex]

  • Copyright holder: Simons Foundation

  • the basic tests are passing (as much as I could run locally)

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@bbbales2
Copy link
Member

@bob-carpenter Over 30 warnings and a successful build causes this to be marked unstable. And this blocks the merge -- it's not that anything failed.

We could also mark these warnings as okay, but I'd like to reproduce the warnings first before we do that. I have a Windows install on this laptop so I can just hop over and investigate.

I'm suspicious that these warnings are only showing up here. Cause we're definitely testing this call in rev. Seems fishy, and like there's something unrelated going on, in which case the warnings might be okay to ignore (or we figure out why this is different and solve them).

@bbbales2
Copy link
Member

@mitzimorris I don't think those are the problem. Nothing is explicitly blowing up. What is happening is that there are too many warnings when building and this is causing the test to be marked unstable.

I suspect this is a problem with the specific test that is generating all these warnings. I will investigate it this afternoon.

@serban-nicusor-toptal
Copy link
Contributor

serban-nicusor-toptal commented Mar 27, 2020

Please see my above comment on how to get to the below links.
Jenkins used this plugin to check, set and verify quality gates.

As a quality gate it was decided to not let more than 30 warnings go to production. ( develop, when we merge this PR in then master on release )

For our build here, we have the following warnings:
GNU C Compiler Warnings
LLVM/Clang Warnings

This job is NOT failing anywhere, everything is building fine, there are no errors! Everything is green and passing!!!

This job is UNSTABLE (yellow) because the Quality Gate was triggered, we have more than the limit of 30 warnings. See the code definition here

I am by no means in the position to just move the quality gate to 999999 and pass this PR.
Recommended it would be to stick to the quality gate and reduce our warnings that we generate, else we can discuss and vote if we should remove the quality gate, raise it or take any other action.

I hope this clarifies the current situation of this PR, if not please tell me so we can all be on the same page. Thank you!


Example of warnings generated from a stan develop build:

GNU C Compiler Warnings
LLVM/Clang Warnings

Develop generated:

16 warnings for GNU C Compiler Warnings while this PR generates 73.
16 warnings for LLVM/Clang Warnings while this PR generates 19.

To stop this build from failing and follow the current CI/CD rules or the quality gate we need to reduce those 73 errors to < 30.


TL;DR

The build is actually fine, there are no errors in the pipeline. What happens is that this PR generated more warnings than allowed by the pipeline as seen here:

GNU C Compiler Warnings
LLVM/Clang Warnings

Options that I can see are:

  1. Try to reduce the warnings to respect the quality gate.
  2. Increase the quality gate. ( This should be decided by a head developer I think ? )
  3. Ignore at all the quality gate and just remove it. ( I would be against it, we may come to a point with lots of warnings that may lead to even broken functionality )

Maybe @seantalts or @jgabry has a saying in the Quality gate threshold.

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 4.88 4.71 1.04 3.59% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.96 -4.21% slower
eight_schools/eight_schools.stan 0.09 0.09 1.02 1.76% faster
gp_regr/gp_regr.stan 0.22 0.22 0.99 -1.3% slower
irt_2pl/irt_2pl.stan 6.45 6.45 1.0 0.01% faster
performance.compilation 89.0 86.78 1.03 2.49% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 7.6 7.57 1.0 0.35% faster
pkpd/one_comp_mm_elim_abs.stan 20.27 21.1 0.96 -4.08% slower
sir/sir.stan 94.66 93.42 1.01 1.31% faster
gp_regr/gen_gp_data.stan 0.05 0.05 1.0 0.39% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 2.95 2.96 1.0 -0.36% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.31 0.32 0.97 -2.88% slower
arK/arK.stan 1.74 1.73 1.0 0.45% faster
arma/arma.stan 0.65 0.66 1.0 -0.37% slower
garch/garch.stan 0.51 0.51 1.0 -0.11% slower
Mean result: 0.998476460516

Jenkins Console Log
Blue Ocean
Commit hash: b4bccc9


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@bob-carpenter
Copy link
Contributor Author

Please see my above comment on how to get to the below links.

Thanks. That's super helpful. Sorry to be so dense here but the green lights all confused me, so I wasn't looking on that page for errors, despite the whole thing being yellow-orange and you having told me to look there before.

Now I just need to work on recreating locally, because unlike the last ones, these are issues I can't tickle on my own machine. I might be able to guess at fixes, which is the next step if I can't get a login to a machine where I can recreate the issue directly.

This job is UNSTABLE (yellow) because the Quality Gate was triggered, we have more than the limit of 30 warnings

Right. @bbbales2 pointed that out and I see what's going on and can now see the actual warnings that are triggering the failed tests (if not a failed build per se).

I want to reserve turning up warning threshold as a last resort. Hopefully we can fix this with some platform-specific defs somewhere.

@serban-nicusor-toptal
Copy link
Contributor

You're very welcome, I'm here to help you out so don't hesitate to ask!
If you need me to spin up a separate machine so you can work/test on just let me know.

@bob-carpenter
Copy link
Contributor Author

It must be failing on pow(double, int) (which happens in write_array), because pow(var, int) couldn't match pow(double, double).

The issue is that pow(double, int) is unambiguous in C++11, which added specific overloads for all pairs of arithmetic types. That means there should be a built-in that matches this exactly. So as far as I can tell, this is a bug in the mingw compiler.

If someone knows how to write a preprocessor conditional (#ifdef or whatever) that matches just this compiler, we can add a pow(double, int) specialization in stan::math in stan/math/prim/fun/pow.hpp, which will match better than either of the two candidates it's currently considering. We can't just add that specialization or it'll lead to ambiguity for all the other compilers. And we can't replace the var with a template parameter that requires var and disallows int or we won't be specific enough to outmatch the built-ins (which don't work across compilers) elsewhere.

@bbbales2 Any headway? You'd asked me not to work on this until you had a chance to look at it.

@bbbales2
Copy link
Member

If someone knows how to write a preprocessor conditional (#ifdef or whatever) that matches just this compiler, we can add a pow(double, int) specialization in stan::math in stan/math/prim/fun/pow.hpp

You've stolen my glory booo. I just got back from the Wild World of Windows and adding:

double pow(double x, int y) {
  return pow(x, double(y));
}

to stan/math/prim/fun/pow.hpp (in the stan::math namespace) is the thing that worked. The math.h there only defines pow for all doubles.

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Mar 27, 2020

@bbbales2 : How did you find that output you showed that said what the ambiguities were? I can't find it anywhere on Jenkins. @serban-nicusor-toptal's instructions only indicated the top-level cause of failure---too many warnings. Then @mitzimorris and I spent another 15m hunting for where the output could've come from and found a hint one level down from where @serban-nicusor-toptal pointed us. You need to click on the warnings icon highlighted, but then I wouldn't have known to look in rosenbrock.hpp unless @bbbales2 had sent me the clue. Otherwise, there are lots and lots of reports here behind tiny buttons, each of which gives you a teasing amount of text. For instance, it says the line of code that's ambiguous, but not what the ambiguity is.

I'm sorry to have to keep repeating my question, but I still don't have an answer that I can work from:

Is there any way to get the console output from the warnings that are causing failure?

Thanks. And sorry for shouting.

@bbbales2
Copy link
Member

Let's zoom and I'll show you. I'll e-mail you a link

@SteveBronder
Copy link
Collaborator

If someone knows how to write a preprocessor conditional (#ifdef or whatever) that matches just this compiler, we can add a pow(double, int) specialization

From below it looks like you can use __MINGW32__ etc and then __MINGW32_MAJOR_VERSION etc. to check if the mingw installed needs the definition. idk if we have the resources to check later versions for this but but worst case we can set it to the version we run tests against

https://blog.kowalczyk.info/article/j/guide-to-predefined-macros-in-c-compilers-gcc-clang-msvc-etc..html

@mitzimorris
Copy link
Member

two more steps after Nic's excellent instructions above -

  1. We are now in the Jenkins classic view, on the left you will find GNU C Compiler Warnings or LLVM/Clang Warnings.
  • click on the apropriate compiler warnings icon

  • in the page displayed, add the following to the URL: consoleText

@bbbales2
Copy link
Member

@bob-carpenter I made the new pow inline. It was failing in the translation unit tests (multiple symbols or something)

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 4.83 4.84 1.0 -0.14% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.97 -2.88% slower
eight_schools/eight_schools.stan 0.09 0.09 1.03 2.58% faster
gp_regr/gp_regr.stan 0.22 0.22 1.0 -0.11% slower
irt_2pl/irt_2pl.stan 6.43 6.46 1.0 -0.5% slower
performance.compilation 87.99 86.86 1.01 1.28% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 7.52 7.53 1.0 -0.11% slower
pkpd/one_comp_mm_elim_abs.stan 20.18 21.1 0.96 -4.52% slower
sir/sir.stan 91.06 91.0 1.0 0.07% faster
gp_regr/gen_gp_data.stan 0.05 0.05 1.01 0.55% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 2.96 2.95 1.0 0.13% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.31 0.33 0.93 -7.17% slower
arK/arK.stan 1.74 1.75 0.99 -0.79% slower
arma/arma.stan 0.66 0.66 1.0 0.14% faster
garch/garch.stan 0.51 0.52 0.99 -0.8% slower
Mean result: 0.992396761765

Jenkins Console Log
Blue Ocean
Commit hash: 1fce197


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@bbbales2 bbbales2 merged commit ea2d135 into develop Mar 28, 2020
@bob-carpenter
Copy link
Contributor Author

Thanks---finally something to celebrate!

I didn't see your fix, so just pushed the same one to the dead branch :-)

Now it's onto matrices and FFTs.

@bob-carpenter
Copy link
Contributor Author

You've stolen my glory booo.

I would've said "great minds think alike." More seriously, this was a huge group effort from beginning to end. In retrospect, I'm not even sure it was worth as much of my time as I spent on it. I'm thinking this was something like two solid months of effort spread over something like five months. This one I very much mis-estimated based on an assumption that the compilers would implement the spec.

@bob-carpenter bob-carpenter deleted the feature/0123-complex-funs branch March 28, 2020 14:52
@SteveBronder
Copy link
Collaborator

Do we want to include these at the language level? I think it would be pretty easy to add a complex scalar / array type, dunno how we would want to do matrices

@bob-carpenter
Copy link
Contributor Author

Yes, and it might be a good project to start the scalar complex numbers before sparse matrices because the I/O is soooo much simpler.

For matrices, we'd need new complex_matrix, complex_vector and complex_row_vector types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants