Remove the batch dimensions from the SplineBuilder and SplineEvaluator class templates #792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove the batch dimensions in class template of the SplineBuilder and SplineBuilder2D classes, and template the operator() instead - Update the relevant tests and examples accordingly
- Remove the batch dimensions from the SplineEvaluator class template - Template the operator() and eval/deriv and integrate functions over the batch dimensions (the batched dims cannot be deduced from the arguments of the integrate function)
- The use of variadic templates for the batch dimensions caused issue when compiling with nvcc - Replaced the variadic template on the dimensions by a single template argument for the corresponding DiscreteDomain
- Update the splines benchmark code - Fix formatting of a few examples - Fix a documentation typo in one of SplineEvaluator2D's constructor
- Add tests where a SplineBuilder and a SplineEvaluator are reused for another interpolation with a different batch pattern - Change the tests, example and benchmark to use the SplineBuilder constructor taking the interpolation domain as argument
- Make one of the variables const as reported by clang-tidy
- Remove the hosts test for the MultipleBatchDomain tests - Remove the high dimension (3D, 4D) tests for the 1D MultipleBatchDomain tests
tpadioleau
requested changes
Mar 6, 2025
There was a problem hiding this comment.
This looks very good. You did only the necessary changes and that helps a lot to review.
For this PR I suggest:
- we rename where it makes sense some templates that represent discrete domains, for example:
DDom->BatchedInterpolationDDomDDom->BatchedDDomBatchDDom->BatchedDDom
- you add your name to the contributors
For a future PR I suggest:
- we deprecate the possibility to pass
ddc::ChunkSpan<Coordinate<CoordsDims...>>for the spline evaluator, only keepddc::ChunkSpan<Coordinate<CDim>> - we deprecate
batched_interpolation_domain_typeand the corresponding functionbatched_interpolation_domain(DDom const& ...)from spline builders - we deprecate
SplineBuilderconstructors that take a batched domain as input parameters, we let the user pass only the interpolation domain - we continue working on the readability of the spline tests
| { | ||
| return ddc::remove_dims_of(batched_interpolation_domain(), interpolation_domain()); | ||
| return ddc::remove_dims_of(batched_interpolation_domain, interpolation_domain()); |
There was a problem hiding this comment.
Suggested change
| return ddc::remove_dims_of(batched_interpolation_domain, interpolation_domain()); | |
| assert(interpolation_domain() == interpolation_domain_type(batched_interpolation_domain)); | |
| return ddc::remove_dims_of(batched_interpolation_domain, interpolation_domain()); |
I suggest that we add an assertion to check the user is still manipulating a batched_interpolation_domain with the same interpolation_domain used at the construction of the SplineBuilder.
| { | ||
| return ddc::replace_dim_of< | ||
| interpolation_discrete_dimension_type, | ||
| bsplines_type>(batched_interpolation_domain(), spline_domain()); | ||
| bsplines_type>(batched_interpolation_domain, spline_domain()); |
There was a problem hiding this comment.
Same remark as for the batch_domain(...) function
| Layout, | ||
| memory_space>> const derivs_xmax) const | ||
| { | ||
| auto const batched_interpolation_domain = vals.domain(); | ||
|
|
||
| assert(interpolation_domain_type(batched_interpolation_domain) == m_interpolation_domain); |
| DDom const& batched_interpolation_domain, | ||
| std::optional<std::size_t> cols_per_chunk = std::nullopt, | ||
| std::optional<unsigned int> preconditioner_max_block_size = std::nullopt) | ||
| : SplineBuilder( |
There was a problem hiding this comment.
Good to use constructor delegation
| { | ||
| return m_spline_builder1.batched_interpolation_domain(); | ||
| return batched_interpolation_domain; |
| { | ||
| return ddc::remove_dims_of(batched_interpolation_domain(), interpolation_domain()); | ||
| return ddc::remove_dims_of(batched_interpolation_domain, interpolation_domain()); |
| batched_spline_domain_type batched_spline_domain() const noexcept | ||
| template <class DDom, class = std::enable_if_t<ddc::is_discrete_domain_v<DDom>>> | ||
| batched_spline_domain_type<DDom> batched_spline_domain( | ||
| DDom const& batched_interpolation_domain) const noexcept | ||
| { | ||
| return ddc::replace_dim_of<interpolation_discrete_dimension_type1, bsplines_type1>( |
| @@ -453,13 +465,21 @@ class SplineEvaluator | |||
| * points represented by this domain are unused and irrelevant. | |||
| * @param[in] spline_coef A ChunkSpan storing the spline coefficients. | |||
| */ | |||
| template <class Layout1, class Layout2> | |||
| template <class Layout1, class Layout2, class BatchedSplineDDom, class BatchDDom> | |||
There was a problem hiding this comment.
Suggested change
| template <class Layout1, class Layout2, class BatchedSplineDDom, class BatchDDom> | |
| template <class Layout1, class Layout2, class BatchedSplineDDom, class BatchedDDom> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses #668
The batch dimensions are removed from the class templates of
SplineBuilder,SplineBuilder2D,SplineEvaluatorandSplineEvaluator2D, theoperator(), relevant type aliases and methods are templated on the batched interpolation/evaluation domain instead. Some methods of theSplineBuilderclasses now need the batched interpolation domain to be passed as argument.