| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Also after this release I'm going to write a script that we can use in jenkins to check that includes are being done correctly |
Sorry, something went wrong.
|
Tagging #3006 just so this PR shows up in the history of that issue -- as you say, actually resolving it would require Eigen to make some changes first |
Sorry, something went wrong.
|
Running with latest compilers: https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FBleedingEdgeCompilersMonthly/detail/BleedingEdgeCompilersMonthly/235/pipeline |
Sorry, something went wrong.
There was a problem hiding this comment.
A bunch of tiny questions/comments -- thanks for tackling this!
Sorry, something went wrong.
There was a problem hiding this comment.
A few small things in the new fwd changes, plus the above comments on doc seem to be outstanding
Sorry, something went wrong.
| #include <stan/math/prim/fun/to_ref.hpp> | ||
| #include <stan/math/prim/fun/transpose.hpp> | ||
| #include <stan/math/fwd/fun/multiply.hpp> | ||
| #include <stan/math/prim/fun/multiply.hpp> |
There was a problem hiding this comment.
Was this meant to be a deletion?
Sorry, something went wrong.
There was a problem hiding this comment.
No sorry we are supposed to use multiply in this function.
multiply returns a scalar for multiply(vector, row_vector). Should this function also return a scalar in the case of a vector?
Sorry, something went wrong.
There was a problem hiding this comment.
We’re including the fwd multiply higher up
Sorry, something went wrong.
There was a problem hiding this comment.
^yes. What about the second part of my Q. I think it should return the same thing as multiply?
Sorry, something went wrong.
There was a problem hiding this comment.
I think this should always return a Eigen::Matrix to match prim, rev, etc. so the latest looks correct?
Sorry, something went wrong.
Jenkins Console Log Machine information No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: G++: Clang: |
Sorry, something went wrong.
|
@SteveBronder I addressed all my own docstring comments, so it's just the ~6 things above that are unresolved, including ones from my original review which are unfortunately "hidden" by github and you need to click to expand |
Sorry, something went wrong.
Jenkins Console Log Machine information No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: G++: Clang: |
Sorry, something went wrong.
Jenkins Console Log Machine information No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: G++: Clang: |
Sorry, something went wrong.
|
@SteveBronder final check -- good to merge? |
Sorry, something went wrong.
Jenkins Console Log Machine information No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: G++: Clang: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
For the last few releases Stan math has had to do quick fixes when clang would update their functions for 'std::complex' which can cause conflicts when looking for our own functions that work over complex. The main issue for function calls like the following where an unqualified name lookup is used to find the correct function to call.
In the above, sin is looked for in both the stan::math namespace and the std namespace since unqualified name lookup also uses Argument Dependent Lookup (ADL). During ADL the compiler looks at the namespaces of the arguments as well as the local namespace to figure out all of the valid function signatures it should use when deciding which function is the best candidate. Since we override std::complex the std namespace is then also included in the valid candidates. Sometimes this is fine because the clang standard library uses requires very similar to ours for only supporting floating point types inside of their complex functions.
But some functions, like the one below (linked here), do not restrict themselves to only floating point types.
This causes issues because ADL then finds two valid candidates functions when trying to find the best candidate for conj (both the stan::math version and the std version).
There is no good solution to this issue, it's just how the language is defined. The only way to make stan::math functions a better candidate is to use templates to make the stan math function both more generic and have an extra template with our requires which makes it slightly better so the compiler chooses it as the best candidate.
So this PR does a few things.
Tests
No changes to tests
Side Effects
Hopefully not
Release notes
Template complex functions in the Stan math library to be better candidates than the standard library functions during ADL.
Checklist
Copyright holder: Simons Foundation
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested