Skip to content
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

feat: use policy-based error handling in interpolators #1245

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fpelliccioni
Copy link

This PR updates cubic_hermite, makima, and pchip interpolators to use boost::math::policies::raise_domain_error instead of throwing exceptions.
This aligns their error handling with Boost.Math’s policy-based approach.

Related PR

#1244 depends on this PR.

Summary

Replaces exceptions with policy-based error handling.
Ensures compatibility with compile-time tests where exceptions are disabled.

Next Steps

Once merged, #1244 will be unblocked and ready for review.

@fpelliccioni fpelliccioni force-pushed the feat/interpolator-error-handling branch from 6a26f57 to 6e670de Compare February 12, 2025 16:50
@mborland
Copy link
Member

I'm not a fan of changing the API to add policy support unless you have a compelling reason. I think this could simply have replaced the current throw std::whatever to using the boost policy for signaling errors.

I feel like this is un-ergonomic:

   auto ret = boost::math::interpolators::pchip<std::vector<double>>::create(std::move(data_x), std::move(data_y), 3, 2);
   check_result<double>(ret.first->operator()(1.0));

compared to:

boost::math::interpolators::pchip<std::vector<double>> s(std::move(data_x), std::move(data_y), 3, 2);
   check_result<double>(s(1.0));

CC: @NAThompson

@fpelliccioni fpelliccioni force-pushed the feat/interpolator-error-handling branch from 6e670de to 48aafd1 Compare February 13, 2025 16:42
Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot more than the first iteration.

@fpelliccioni fpelliccioni force-pushed the feat/interpolator-error-handling branch 2 times, most recently from 97c78c8 to 949aeb5 Compare February 13, 2025 18:26
@fpelliccioni fpelliccioni force-pushed the feat/interpolator-error-handling branch from 949aeb5 to c3ab311 Compare February 13, 2025 18:41
Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. I kicked off the CI run to make sure it agrees.

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.

3 participants