-
Notifications
You must be signed in to change notification settings - Fork 69
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
Templatize on the need for the integral of the third kind #2362
Conversation
|
||
std::mt19937_64 random(42); | ||
std::uniform_real_distribution<> distribution_φ(0.0, π / 2); | ||
std::uniform_real_distribution<> distribution_n(0.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the n's and the loop at line 40 is probably leading to too efficient caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
numerics/elliptic_integrals.cpp
Outdated
}; | ||
|
||
template<typename T, typename = EnableIfAngleResult<T>> | ||
inline constexpr bool should_compute = !std::is_same_v<T, UnusedResult const>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we believe that template variables work on Mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work.
template<typename T, typename = EnableIfAngleResult<T>> | ||
inline constexpr bool should_compute = !std::is_same_v<T, UnusedResult const>; | ||
|
||
inline constexpr UnusedResult unused{uninitialized}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for inline variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer.
template<typename T> | ||
using EnableIfAngleResult = | ||
std::enable_if_t<std::is_same_v<T, UnusedResult const> || | ||
std::is_same_v<T, Angle>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reorder things a bit for ease of readability:
UnusedResult
,unused
,EnableIfAngleResult
,should_compute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. That also makes it legal, MSVC is overly lenient as usual.
Angle& B_φǀm, | ||
Angle& D_φǀm, | ||
Angle& J_φ_nǀm) { | ||
return FukushimaEllipticBDJ<Angle>(φ, n, mc, B_φǀm, D_φǀm, J_φ_nǀm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the specification of Angle
necessary? I would have expected that it would deduce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, but the non-templatized FukushimaEllipticBDJ
(which we are defining) has precedence here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be to expose UnusedResult
in base
, and to only have the templatized version (declared in the header, with both explicit instantiation declarations).
F_φǀm = B + D; | ||
E_φǀm = B + mc * D; | ||
Π_φ_nǀm = F_φǀm + n * J; | ||
EllipticFEΠ<Angle>(φ, n, mc, F_φǀm, E_φǀm, Π_φ_nǀm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer.
This makes it possible to compute F and E without computing J when Π is not needed.
Before:
After: