Fix pow(a,b) overload resolution under llvm19 #3110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
From jenkins
The recent changes are causing the prim pow to be the valid overload. template <typename T1, typename T2,
require_all_t<
disjunction<is_complex<T1>, std::is_arithmetic<T1>>,
disjunction<is_complex<T2>, std::is_arithmetic<T2>>>* = nullptr>
inline auto pow(const T1& a, const T2& b) {
return std::pow(a, b);
}It looks like we are trying to just have one overload for combinations of std complex and arithmetic types. I think we should break this up so that's it is easier to read. It's going to be a lot more expensive for the compiler though template <typename T1, typename T2,
require_arithmetic_t<T1>* = nullptr, require_arithmetic_t<T2>* = nullptr>
inline auto pow(const std::complex<T1>& a, const std::complex<T2>& b) {
return std::pow(a, b);
}
template <typename T1, typename T2,
require_arithmetic_t<T1>* = nullptr, require_arithmetic_t<T2>* = nullptr>
inline auto pow(const T1& a, const std::complex<T2>& b) {
return std::pow(a, b);
}
template <typename T1, typename T2,
require_arithmetic_t<T1>* = nullptr, require_arithmetic_t<T2>* = nullptr>
inline auto pow(const std::complex<T1>& a, const T2& b) {
return std::pow(a, b);
}
template <typename T1, typename T2,
require_arithmetic_t<T1>* = nullptr, require_arithmetic_t<T2>* = nullptr>
inline auto pow(const T1& a, const T2& b) {
return std::pow(a, b);
}
|
|
@SteveBronder that change didn't fix the expression tests: Details
|
Fix expr tests so they can work with python > 3
|
There are a ton of changes in 836a5ef, but I think they fix the problem and simplify the code a lot. I got rid of a bunch of the overloads and use a constexpr if inside of the functions for some cases like complex types. The one huge change is that I put rev in front of fwd for mix.hpp. I think that makes sense because we want fwd to have access to rev? The tests would not compile if I did not do that |
|
@SteveBronder it looks like some merge conflicts got checked in I’m not sure about the include order, it sounds kind of like the opposite of stan-dev/stan#3294 where we moved rev after fwd? |
4 tasks
|
I need to fix up the docs and then this is good to merge |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
Latest commit passed on clang 19 (https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FBleedingEdgeCompilersMonthly/detail/BleedingEdgeCompilersMonthly/222/pipeline/171) thanks @serban-nicusor-toptal for helping with the CI pipeline debugging |
WardBrian
commented
Sep 30, 2024
There was a problem hiding this comment.
I can't approve a PR I myself opened, but this looks good @SteveBronder
SteveBronder
approved these changes
Sep 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3106. This fixes both the test failures we were seeing in
test/unit/mixand the model compilation that was failing.Two sets of changes were needed:
const &to some template parameters such that our overloads are prioritized (recommended by @SteveBronder)prim/andrev/The second one here was trickier, but necessary to allow vectorized calls to see the
revsignatures. So far as @SteveBronder and I can tell, all of the Stan Math functions that use this style of having the apply-scalar overload last in theprimfile could end up in this situation where said overload can never resolve therevspecializations, if the prim file is included in any rev headers and therefore resolved first.Tests
None. I plan on running the bleeding-edge pipeline on this branch before merging.
Side Effects
Could potentially speed up the vectorized signature of
powas it will now use rev specializations. Could also negatively impact compile times due to the extra resolution required.Release notes
Fixed some failures to compile calls to
powwhen using libc++ 19.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
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested