[math][distributions] superfluous checking of parameters?
The boost math distributions check validity of distribution the same parameters multiple times. E.g. in normal.hpp, class normal_distribution, the constructor, normal_distribution(RealType mean = 0, RealType sd = 1) : m_mean(mean), m_sd(sd) { // Default is a 'standard' normal distribution N01. static const char* function = "boost::math::normal_distribution<%1%>::normal_distribution"; RealType result; detail::check_scale(function, sd, &result, Policy()); detail::check_location(function, mean, &result, Policy()); } however, those are *again* checked in e.g. the pdf() non member function inline RealType pdf(const normal_distribution<RealType, Policy>& dist, const RealType& x) { ... if(false == detail::check_scale(function, sd, &result, Policy())) { return result; } ... } Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Thijs (M.A.) van den Berg Sent: Saturday, October 29, 2011 10:56 AM To: boost@lists.boost.org Subject: [boost] [math][distributions] superfluous checking of parameters?
The boost math distributions check validity of distribution the same parameters multiple times.
E.g. in normal.hpp, class normal_distribution, the constructor,
normal_distribution(RealType mean = 0, RealType sd = 1) : m_mean(mean), m_sd(sd) { // Default is a 'standard' normal distribution N01. static const char* function = "boost::math::normal_distribution<%1%>::normal_distribution";
RealType result; detail::check_scale(function, sd, &result, Policy()); detail::check_location(function, mean, &result, Policy()); }
however, those are *again* checked in e.g. the pdf() non member function
inline RealType pdf(const normal_distribution<RealType, Policy>& dist, const RealType& x) { ... if(false == detail::check_scale(function, sd, &result, Policy())) { return result; } ... }
Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
As I recollect, I think we thought there were circumstances when this check wasn't redundant (but I can't remember the exact case. Users changes the distribution parameters?). And because it was believed better to be safe than sorry, and the check was very cheap (at runtime), we kept it that way. Is there any evidence that this is a problem? Paul --- Paul A. Bristow, Prizet Farmhouse, Kendal LA8 8AB UK +44 1539 561830 07714330204 pbristow@hetp.u-net.com
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Thijs (M.A.) van den Berg Sent: Saturday, October 29, 2011 10:56 AM To: boost@lists.boost.org Subject: [boost] [math][distributions] superfluous checking of parameters?
The boost math distributions check validity of distribution the same parameters multiple times.
E.g. in normal.hpp, class normal_distribution, the constructor,
normal_distribution(RealType mean = 0, RealType sd = 1) : m_mean(mean), m_sd(sd) { // Default is a 'standard' normal distribution N01. static const char* function = "boost::math::normal_distribution<%1%>::normal_distribution";
RealType result; detail::check_scale(function, sd, &result, Policy()); detail::check_location(function, mean, &result, Policy()); }
however, those are *again* checked in e.g. the pdf() non member function
inline RealType pdf(const normal_distribution<RealType, Policy>& dist, const RealType& x) { ... if(false == detail::check_scale(function, sd, &result, Policy())) { return result; } ... }
Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
As I recollect, I think we thought there were circumstances when this check wasn't redundant (but I can't remember the exact case. Users changes the distribution parameters?).
And because it was believed better to be safe than sorry, and the check was very cheap (at runtime), we kept it that way.
Is there any evidence that this is a problem?
I hadn't though about the changing parameter user case. That's valid. I don't think it's a problem, I mainly want to learn the design rationale and see if I need to check things in a similar same way in another project I'm working on, ..in which I want to align the design with math.distributions The changing parameter case is a good explanation and something I need to covert too. Thanks.
Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
As I recollect, I think we thought there were circumstances when this check wasn't redundant (but I can't remember the exact case. Users changes the distribution parameters?).
And because it was believed better to be safe than sorry, and the check was very cheap (at runtime), we kept it that way.
The issue is like this: * If the current policy (a template param) is to throw on domain errors (the default), then the second check in the non-member functions is redundant as any errors will have triggered an exception in the distribution constructor. On the other hand: * If the current policy is to not throw (return NaN on domain errors), then the constructor checks are redundant and the non-member function checks are the required ones. If it really is a performance bottleneck, then we could add a check on the current exception policy so that those two mutually exclusive options get optimized at compile time. HTH, John.
Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
As I recollect, I think we thought there were circumstances when this check wasn't redundant (but I can't remember the exact case. Users changes the distribution parameters?).
And because it was believed better to be safe than sorry, and the check was very cheap (at runtime), we kept it that way.
The issue is like this:
* If the current policy (a template param) is to throw on domain errors (the default), then the second check in the non-member functions is redundant as any errors will have triggered an exception in the distribution constructor. On the other hand: * If the current policy is to not throw (return NaN on domain errors), then the constructor checks are redundant and the non-member function checks are the required ones.
If it really is a performance bottleneck, then we could add a check on the current exception policy so that those two mutually exclusive options get optimized at compile time.
HTH, John. Thanks John. Makes perfect sense. It's not a performance issue for me, I was curious about the idea behind it, I didn't see that straight away.
Also, Paul mentioned that distribution parameters might get modified after construction (but thinking a bit more about it, -and looking at the code- I cant see how they can get modified). Thanks both for explaining the motivation, it's clear to me now
Why is that? It's done in a consistent way across all distributions. Can we remove the check inside the non member functions that are already checked in the constructor?
As I recollect, I think we thought there were circumstances when this check wasn't redundant (but I can't remember the exact case. Users changes the distribution parameters?).
And because it was believed better to be safe than sorry, and the check was very cheap (at runtime), we kept it that way.
The issue is like this:
* If the current policy (a template param) is to throw on domain errors (the default), then the second check in the non-member functions is redundant as any errors will have triggered an exception in the distribution constructor. On the other hand: * If the current policy is to not throw (return NaN on domain errors), then the constructor checks are redundant and the non-member function checks are the required ones.
If it really is a performance bottleneck, then we could add a check on the current exception policy so that those two mutually exclusive options get optimized at compile time.
From a performance point of view, suppose I wanted to have a Policy mechanism that allows me to eliminate *all* error checking machine instructions from a distribution constructor of distribution non member function. Would it be possible to move the detail::check_scale(d) etc into the Policy class? That way I could provide a different policy that get's eliminated compile time via an implementation like "inline policy::check_scale(d){return;}"
* If the current policy (a template param) is to throw on domain errors (the default), then the second check in the non-member functions is redundant as any errors will have triggered an exception in the distribution constructor. On the other hand: * If the current policy is to not throw (return NaN on domain errors), then the constructor checks are redundant and the non-member function checks are the required ones.
If it really is a performance bottleneck, then we could add a check on the current exception policy so that those two mutually exclusive options get optimized at compile time.
From a performance point of view, suppose I wanted to have a Policy mechanism that allows me to eliminate *all* error checking machine instructions from a distribution constructor of distribution non member function.
Would it be possible to move the detail::check_scale(d) etc into the Policy class? That way I could provide a different policy that get's eliminated compile time via an implementation like "inline policy::check_scale(d){return;}"
I don't think we could move all the checks inside the policy class as each distribution is more or less different, but we could pass a compile time param to the checking functions to have the same effect. In fact can you file a bug report so I don't forget? Cheers, John.
* If the current policy (a template param) is to throw on domain errors (the default), then the second check in the non-member functions is redundant as any errors will have triggered an exception in the distribution constructor. On the other hand: * If the current policy is to not throw (return NaN on domain errors), then the constructor checks are redundant and the non-member function checks are the required ones.
If it really is a performance bottleneck, then we could add a check on the current exception policy so that those two mutually exclusive options get optimized at compile time.
From a performance point of view, suppose I wanted to have a Policy mechanism that allows me to eliminate *all* error checking machine instructions from a distribution constructor of distribution non member function.
Would it be possible to move the detail::check_scale(d) etc into the Policy class? That way I could provide a different policy that get's eliminated compile time via an implementation like "inline policy::check_scale(d){return;}"
I don't think we could move all the checks inside the policy class as each distribution is more or less different, but we could pass a compile time param to the checking functions to have the same effect. In fact can you file a bug report so I don't forget?
Cheers, John.
Thanks, I will file that. A good solution, I'm going to adapt that too
participants (3)
-
John Maddock -
Paul A. Bristow -
Thijs (M.A.) van den Berg