Skip to content

add asserts in IdentityInterpolationBuilder and deduplicate knot_grid in LagrangeEvaluator#611

Closed
Quntized wants to merge 8 commits into
gyselax:develfrom
Quntized:add_slagrange
Closed

add asserts in IdentityInterpolationBuilder and deduplicate knot_grid in LagrangeEvaluator#611
Quntized wants to merge 8 commits into
gyselax:develfrom
Quntized:add_slagrange

Conversation

@Quntized

Copy link
Copy Markdown
Contributor

Added defensive asserts in IdentityInterpolationBuilder::operator():

Check that derivative arguments are not silently ignored
Validate size match between coeffs and vals before deep_copy
Added missing #include

Deduplicated knot_grid type alias in LagrangeEvaluator:

Moved to class-level private type alias
Removed identical definitions from eval_no_bc and getclosest


Please complete the checklist to ensure that all tasks are completed before marking your pull request as ready for review.

All Submissions

  • Have you ensured that all lines changed in this PR are justified by a comment found in the description ?
  • Have you updated the CHANGELOG.md ?
  • Have you linked any issues that should be closed when this PR is merged (using closing keywords) ?
  • Have you checked that the AUTHORS file is up to date ?
  • Have you checked that the copyright information in the LICENCE file is up to date (including dates) ?
  • Do you follow the conventions specified in our coding standards ?

New Feature Submissions

  • Have you added tests for the new functionalities ?
  • Have you documented the new functionalities:
    • API documentation describing the available methods, when each should be used and how to use them ?
    • User-friendly documentation in README files (which may link to the API documentation).
    • If the new functionality is non-trivial to use, provide a tutorial or example ? (optional)

Changes to Existing Features

  • Have you checked that existing tests cover all code after the changes ?
  • Have you checked that existing tests are still passing ?
  • Have you checked that the existing documentation is still accurate (API and README files) ?

Changes to the CI

  • Have you made the same changes to both the GitHub CI and the GitLab CI (for the private fork) ?

= std::nullopt) const
{
assert(!derivs_xmin.has_value() || derivs_xmin->size() == 0);
assert(!derivs_xmax.has_value() || derivs_xmax->size() == 0);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seeing it's unused that's why I added , is that necessary?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is unused by design but is provided to make this Builder interchangeable with other builders (in particular the SplineBuilder) the size of the derivatives should be collected from the class though so the size should be correct unless derivatives are reused from a previous calculation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation. That makes sense — since the derivative domains come from the builder itself via batched_derivs_xmin_domain(), the size is guaranteed to be correct in normal usage. Removed.

Comment thread src/interpolation/identity_interpolation_builder.hpp Outdated
@Quntized

Copy link
Copy Markdown
Contributor Author

I think everything's fine. I am closing this. If I find anything major issue, I will reopen it. Thank you for your time.

@Quntized Quntized closed this Mar 30, 2026
@Quntized Quntized deleted the add_slagrange branch March 30, 2026 18:17
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