Skip to content

Add manual promotion to arcsine, beta, and Bernoulli distributions#1299

Closed
mborland wants to merge 4 commits intodevelopfrom
dist_audit
Closed

Add manual promotion to arcsine, beta, and Bernoulli distributions#1299
mborland wants to merge 4 commits intodevelopfrom
dist_audit

Conversation

@mborland
Copy link
Member

This is an extension to #1294 since it looks like most of the distributions have functions where manual promotion needs to be applied to follow what the policy says. I tried to reduce the ugliness as much as possible, but the result is that the code is generally less readable than it is without this PR. Thoughts @jzmaddock and @WarrenWeckesser. If there's no real opposition I can continue plugging along with this.

@codecov
Copy link

codecov bot commented Aug 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.47%. Comparing base (483f36c) to head (2852b4b).
⚠️ Report is 275 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1299      +/-   ##
===========================================
- Coverage    95.07%   94.47%   -0.60%     
===========================================
  Files          796      686     -110     
  Lines        67020    49438   -17582     
===========================================
- Hits         63717    46705   -17012     
+ Misses        3303     2733     -570     
Files with missing lines Coverage Δ
include/boost/math/distributions/beta.hpp 77.27% <ø> (-8.16%) ⬇️
include/boost/math/policies/policy.hpp 95.83% <ø> (ø)

... and 138 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 483f36c...2852b4b. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JacobHass8
Copy link
Contributor

JacobHass8 commented Mar 4, 2026

I tried to reduce the ugliness as much as possible, but the result is that the code is generally less readable than it is without this PR. Thoughts @jzmaddock and @WarrenWeckesser. If there's no real opposition I can continue plugging along with this.

Would it be a little prettier if all the computations were done in a separate function. This is seems to be how type promotion is done with special functions. It seems a little more human readable to me since all the promotions are handled in one function and the computations in another. For example, the mean of the arcsin function would be something like this

template <class RealType, class Policy>
BOOST_MATH_GPU_ENABLED inline RealType mean(const arcsine_distribution<RealType, Policy>& dist)
{ // Mean of arcsine distribution .
  RealType result;
  RealType x_min = dist.x_min();
  RealType x_max = dist.x_max();

  if (false == arcsine_detail::check_dist(
    "boost::math::mean(arcsine_distribution<%1%> const&, %1% )",
    x_min,
    x_max,
    &result, Policy())
    )
  {
    return result;
  }

  using policy_promoted_type = policies::evaluation_t<RealType, Policy>;

  return static_cast<RealType>(arcsine_mean(static_cast<policy_promoted_type>(x_min), static_cast<policy_promoted_type>(x_max)));
}

template <class RealType>
inline RealType arcsine_mean(const RealType x_min, const RealType x_max)
{
  return (x_max + x_min) / 2;
}

For the arcsine distribution, this isn't that big of a change. However, it might make things prettier for more complicated functions.

If this seems reasonable, I can take a hack at it.

@mborland
Copy link
Member Author

mborland commented Mar 5, 2026

That does seem like a more elegant solution to me. If you'd like to take a stab at it, be my guest.

@JacobHass8
Copy link
Contributor

That does seem like a more elegant solution to me. If you'd like to take a stab at it, be my guest.

Great! Should I make a new PR? I don't think I have write access to this PR.

@mborland
Copy link
Member Author

mborland commented Mar 5, 2026

That does seem like a more elegant solution to me. If you'd like to take a stab at it, be my guest.

Great! Should I make a new PR? I don't think I have write access to this PR.

Yes. I will close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants