-
Notifications
You must be signed in to change notification settings - Fork 226
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
improve promotion.hpp
#1021
improve promotion.hpp
#1021
Conversation
This all looks good to me, and is pretty much how I would rewrite things here had I the time ;) Some quick comments:
|
A promotion system should be able to be structured in this way. And, this is where I think this promotion system should eventually end up. But it's not currently possible to do this due to the promotion system being used for four different effects in Boost Math.
IMO, these 4 operations should be handled in 4 different parts of the code
To address the previous comments:
I think this issue is relatively minor compared to other issues
I think I fixed the test issues in the latest commit. CI will have the final say.
They looked fine on my end. This refactor should not effect multiprecision promotion. Multiprecion types are not floating point types according to std::is_floating_point. So, their promotion uses the same asymmetric conversion rule as before. |
Previous promotion behavior on the policy side only ever applied promotions to types math/include/boost/math/policies/policy.hpp Lines 732 to 742 in 74bb4bc
This limitation meant that special math functions requiring internal promotion didn't work as intended for This fix seems to have addressed the previous issues with |
On a related note, there appear to be no specializations of math/include/boost/math/differentiation/autodiff.hpp Lines 2003 to 2028 in 74bb4bc
Hopefully specializing promote_args<T1,T2> for autodiff types restores agreement.
|
Matt and I talked about this at length: the conclusion was that if the user is deliberately using a fixed width floating point type then we should respect that and not carry out any internal promotion. In contrast the float/double/long double types are of unspecified width, so playing a little fast and loose with those to ensure internal accuracy seemed sensible. That not withstanding, the speed hit in promoting from double->long double has changed enormously since the library was written, so it may be that we'll change the default to not promote at some point.
Again, wouldn't simply using common_type internally in all cases solve that?
It's also what the behaviour that the standard specifies.
It would be relatively easy to add a "minimum evaluation type" to the policy code, but again, I'm not sure this matters that much as float16_t really shouldn't be used for calculation purposes anyway IMO. |
According to cppreference:
Given that 64 bit support is ubiquitous, I think the vast majority of users are going to expect I think that user's type expectations are primarily concerned with input/output relationships and not internal promotion. For example, if I call
There are a variety of problems with increasing the use of #include "test_autodiff.hpp"
#include <boost/math/tools/promotion.hpp>
#include <boost/type_index.hpp>
void test_promotion() {
using ft_f_5 = decltype(make_fvar<float, 5>(float(1)));
using ft_d_3 = decltype(make_fvar<double, 3>(double(1)));
using ft_promoted_a = boost::math::tools::promote_args<ft_d_3, ft_f_5>::type;
using ft_promoted_b = std::common_type_t<double, ft_f_5>;
// using ft_promoted_c = std::common_type_t<ft_f_5, ft_d_3>; // Doesn't compile
std::cout << "ft_promoted_a: " << boost::typeindex::type_id<ft_promoted_a>().pretty_name() << "\n";
std::cout << "ft_promoted_b: " << boost::typeindex::type_id<ft_promoted_b>().pretty_name() << "\n";
}; In other places in the code such as math/include/boost/math/special_functions/beta.hpp Lines 1495 to 1502 in 74bb4bc
the thing you are promoting is literally a policy. So trying to use std::common_type here gives you a compile error that says:
../../../boost/math/special_functions/binomial.hpp:73:124: required from here
/usr/include/c++/9/type_traits:2386:11: error: no type named ‘type’ in ‘struct std::common_type<double, boost::math::policies::policy<boost::math::policies::promote_double<false>, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy, boost::math::policies::default_policy> >’
The promotion that occurs when one calls
I think what some people may want is to be able to have their float type of choice be both the input and output types of various functions and for these calculations to be reasonably accurate. They don't want to have to manually convert types to get good answers. If promotion is required internally to maintain reasonable input/output accuracy, this is fine. Minimizing user effort is a design goal. Maybe @gpeterhoff can weigh in here as a |
It seems that supporting internal promotion for types that are not |
I am still not convinced we should be promoting on fixed width types. If there was some floating point version of the fast or least options that integers have that case would be different. Since you brought up
If we start promoting |
Performance and
I'm not either. Things I am convinced of:
Currently when math/include/boost/math/tools/promotion.hpp Line 117 in 74bb4bc
math/include/boost/math/tools/promotion.hpp Line 246 in 74bb4bc
By changing the promotion behavior of |
I don't think
I think we should continue with |
Okay, this is becoming a little more clear. I'm interested in @jzmaddock's opinions on Irrespective of his perspective, what are your opinions on the promotion of autodiff types. For example, if it's policy to promote |
That makes sense to me. I would ask @pulver since my understanding of the mechanics of autodiff are limited. |
From a high level perspective, that makes sense. Some specific use case(s) might help illustrate the issues, if there is some hesitancy/question about this. |
I'm not suggesting we literally replace promote_args with common_type - rather that promote_args becomes a wrapper around common type, and autodiff and other awkward cases (multiprecision expression templates for example) continue to specialize promote_args as now.
That's not so good, it's a bit of lazy C++98'ism on my part, we can do better now, but that's probably beyond the scope of this PR.
Sure, but
Me neither to be honest. With regard to float64 and double (for example), since these are distinct types with presumably distinct uses I don't necessarily see any issues in treating them differently. |
I think you're saying that you would like for
That's fine, perhaps this method could be renamed to
I'm more or less convinced now. The motivation behind policy-side promotion wasn't to enforce a minimum bits size for reasonable accuracy, but rather to get a little bit of extra accuracy for free. Because this isn't true anymore, it doesn't make sense to continue doing this for the floating point types introduced in C++23. Because I didn't understand the motivation behind the policy-side promotion, the changes I made in this PR that affected policy-side promotion are wrong. I'm going to close this PR. I had previously chunked off the parts of this PR not affecting policy-side promotion into #1022. @jzmaddock I can open an issue summarizing the current conversation about policy-side promotion in case others have similar questions if you would like. |
This PR attempts to address some of the issues raised in #1020 including:
float
argumentsThis PR mostly refactors the promotion functionality to make how the system works more clear. Design choices that change the promotion behavior break too many tests and are out of the scope of this PR. Also, this refactor seemed to have no effect on the time required to compile and run the unit tests in Boost Math.