Add 3D Splines (with periodic or greville boundary conditions) #925

tretre91 · 2025-09-30T08:43:07Z

This PR adds the SplineBuilder3D and SplineEvaluator3D classes to handle 3D splines. Some 3D spline tests are also added.
In its current state, Hermite boundary conditions are not handled. Also, the API for the SplineBuilder3D will certainly need to be modified (we currently take as arguments the derivatives on the faces and the cross derivatives on the corners, but we might also want the cross derivatives on the edges).

codecov · 2025-09-30T09:17:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.88%. Comparing base (fc6a29e) to head (75cc670).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #925      +/-   ##
==========================================
- Coverage   92.63%   91.88%   -0.75%     
==========================================
  Files          55       57       +2     
  Lines        2673     2834     +161     
  Branches      839      925      +86     
==========================================
+ Hits         2476     2604     +128     
- Misses        112      136      +24     
- Partials       85       94       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

science-enthusiast · 2025-10-07T12:24:11Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+
+    /// @brief The type of the SplineBuilder used by this class to spline-approximate the third-dimension-derivatives along second dimension.
+    using builder_deriv_type2 = ddc::
+            SplineBuilder<ExecSpace, MemorySpace, BSpline2, DDimI2, BcLower2, BcUpper2, Solver>;


Let's introduce the types builder_deriv_type1 and builder_deriv_type2 in the PR that handles the boundary conditions.

science-enthusiast · 2025-10-08T08:16:27Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+    builder_type2 m_spline_builder2;
+    builder_type3 m_spline_builder3;
+    builder_deriv_type1 m_spline_builder_deriv1;
+    builder_deriv_type2 m_spline_builder_deriv2;


Similarly m_spline_builder_deriv1 and m_spline_builder_deriv2 could be introduced later.

Maybe by not introducing unnecessary variables at this stage, we will also understand better the Spline codebase.

While we are at this, we could also note down small adjustments to be made in spline_builder_2d.hpp:

builder_type1 m_spline_builder1; builder_deriv_type1 m_spline_builder_deriv1; builder_type2 m_spline_builder2;

can be:

builder_type1 m_spline_builder1; builder_type2 m_spline_builder2; builder_deriv_type1 m_spline_builder_deriv1;

science-enthusiast · 2025-10-08T08:40:53Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+    using batched_derivs_domain_type3 = ddc::replace_dim_of_t<
+            BatchedInterpolationDDom,
+            interpolation_discrete_dimension_type3,
+            deriv_type3>;


These types are again needed only in ddc::BoundCond::HERMITE. We could keep them here if it is too much work to retain only the PERIODIC related stuff. I see that the corresponding variables are annotated as [[maybe_unused]].

Also, here and in spline_builder_2d.hpp why cannot the definition of batched_derivs_domain_type1 have the same form as batched_derivs_domain_type2? Like the following:

template < class BatchedInterpolationDDom, class = std::enable_if_t<ddc::is_discrete_domain_v<BatchedInterpolationDDom>>> using batched_derivs_domain_type1 = ddc::replace_dim_of_t< BatchedInterpolationDDom, interpolation_discrete_dimension_type1, deriv_type1>;

science-enthusiast · 2025-10-08T08:47:44Z

tests/splines/CMakeLists.txt

    endforeach()
 endforeach()

+foreach(BC "BC_PERIODIC" "BC_GREVILLE") # "BC_HERMITE")


Just like commenting out BC_HERMITE here, let us see whether we can comment out or remove BC_HERMITE related stuff in all places in CMake and C++ logic. Removing all the #if defined(BC_HERMITE) should be at least easy.

science-enthusiast · 2025-10-08T09:25:07Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+        , m_spline_builder_deriv1(interpolation_domain)
+        , m_spline_builder_deriv2(interpolation_domain)
+    {
+    }


Here and in spline_builder_2d.hpp is this constructor ever called directly or is it always called in a delegated manner by the following constructor?

science-enthusiast · 2025-10-08T09:35:19Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+     * @param x An rvalue to another SplineBuilder.
+     * @return A reference to this object.
+     */
+    SplineBuilder3D& operator=(SplineBuilder3D&& x) = default;


Here and in spline_builder_2d.hpp, the explicitly declared default destructor could be removed. Also, the order can be changed. Explicitly marking both the copy operations as deleted, followed by the defaulted move operations. We could also add a comment that this is a move only type. The dummy variable x is also not needed anywhere.

science-enthusiast · 2025-10-08T09:53:03Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+    {
+        assert(interpolation_domain() == interpolation_domain_type(batched_interpolation_domain));
+        return batched_interpolation_domain;
+    }


Here and in spline_builder_2d.hpp, we could rename this function template as copy_batched_interpolation_domain.

Also, I am not where this being used.

science-enthusiast · 2025-10-08T09:54:33Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+     *
+     * @param batched_interpolation_domain The whole domain on which the interpolation points are defined.
+     *
+     * @return The domain for the interpolation mesh.


Here and in spline_builder_2d.hpp, the return type could be worded as The whole domain for ....

science-enthusiast · 2025-10-08T17:08:44Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+    m_spline_builder2(spline2, spline1.span_cview());
+
+    // Spline3-approximate spline2
+    m_spline_builder3(spline, spline2.span_cview());


Can we have a separate PR where we call SplineBuilder followed by SplineBuilder2D? We will have a SplineBuilder2D member etc, in that implementation. That approach will help with nD approach.

science-enthusiast · 2025-10-08T18:40:30Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+
+    LowerExtrapolationRule3 m_lower_extrap_rule_3;
+
+    UpperExtrapolationRule3 m_upper_extrap_rule_3;


The extra lines inbetween can be removed.

science-enthusiast · 2025-10-08T18:48:56Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+                    ddc::ChunkSpan<
+                            double const,
+                            spline_domain_type,
+                            Kokkos::layout_right,


Can we define an alias (e.g., spline_layout) for Kokkos::layout_right and use it everywhere?

science-enthusiast · 2025-10-08T18:52:33Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+     * @param x An rvalue to another SplineEvaluator.
+     * @return A reference to this object.
+     */
+    SplineEvaluator3D& operator=(SplineEvaluator3D&& x) = default;


We can follow the rule of zero and not define any of them in a defaulted manner.

Not necessary imo.

science-enthusiast · 2025-10-08T19:13:51Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+    template <
+            class Layout1,
+            class Layout2,
+            class Layout3,


In the comments can we say, why we are giving the flexibility of three separate Layouts?

science-enthusiast · 2025-10-09T07:23:52Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+     * @return The derivative of the spline function at the desired coordinate.
+     */
+    template <class Layout, class... CoordsDims>
+    KOKKOS_FUNCTION double deriv_1_and_3(


Shall we move the definition of deriv_1_3 between deriv_1_and_2 and deriv_2_and_3? If we can follow an order in which we list tuples of dimensions, it will help with recursive implementation.

Or 1_2 followed by 2_3 followed by 3_1 (not 1_3, even though there is no technical difference between 1_3 and 3_1).

science-enthusiast · 2025-10-09T07:33:27Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+                || (std::is_same_v<
+                            InterestDim2,
+                            typename evaluation_discrete_dimension_type1::continuous_dimension_type>
+                    && std::is_same_v<InterestDim1, continuous_dimension_type3>));


In these static asserts there is no opportunity to compare with typename evaluation_discrete_dimension_type3::continuous_dimension_type, is that ok?

The same situation is already there in deriv2 in spline_evaluator_2d.hpp. It will be good to understand more about this.

science-enthusiast · 2025-10-09T07:51:22Z

include/ddc/kernels/splines/spline_evaluator_3d.hpp

+                            InterestDim2,
+                            typename evaluation_discrete_dimension_type1::continuous_dimension_type>
+                    && std::is_same_v<InterestDim3, continuous_dimension_type2>
+                    && std::is_same_v<InterestDim1, continuous_dimension_type3>));


Again, the order can be changed. (1, 2, 3) followed by (2, 3, 1) then (3, 1, 2).

EmilyBourne · 2025-10-09T10:42:32Z

include/ddc/kernels/splines/spline_builder_3d.hpp

+    template <class Layout, class BatchedInterpolationDDom>
+    void operator()(


Are different layouts tested?
This may be an additional implementation of this issue:
#886

I don't think so, this PR is the 3D extension of the 2D classes, based on the same tests without Hermite boundary conditions.

In that case the template parameter Layout should probably be removed given that it is probably broken anyway

I was not aware of this issue, but if it is present in 2D, it will also be present in #929 since we settled on a recursive approach using 1D and 2D builders for the 3D builder

I don't know what is the best approach for now. I think currently only layout_right is working fine so if you prefer that the API reflects this we can do a quick fix.

We are starting to play with StridedDiscreteDomain, we think that other layouts will be needed but not clear yet.

- rename some of the type aliases - use only one deriv dimension in the derivs_domain type aliases - enable the greville tests - lower the norm threshold for the triple derivative to 1e-10

parameters

tpadioleau · 2025-10-10T05:23:12Z

As discussed, @science-enthusiast please open an issue listing all your comments so that they can be addressed on all builders/evaluators.

tpadioleau force-pushed the multid_splines branch 2 times, most recently from 3774590 to ec69b64 Compare October 2, 2025 13:05

tretre91 mentioned this pull request Oct 3, 2025

Add 3D Splines #929

Merged

tpadioleau marked this pull request as ready for review October 3, 2025 16:30

tpadioleau force-pushed the multid_splines branch from ec69b64 to 2a01541 Compare October 3, 2025 16:30

science-enthusiast reviewed Oct 8, 2025

View reviewed changes

science-enthusiast reviewed Oct 9, 2025

View reviewed changes

EmilyBourne reviewed Oct 9, 2025

View reviewed changes

tretre91 and others added 9 commits October 10, 2025 07:22

Add a 3D spline builder for the periodic case

4b408a3

Changes to the derivs domain

e9b3232

- rename some of the type aliases - use only one deriv dimension in the derivs_domain type aliases - enable the greville tests - lower the norm threshold for the triple derivative to 1e-10

Reword the FIXMEs in the 3D test

6875f7a

Fix a typo

2edd0b9

Format the splines CMakeLists.txt

ccd338e

Add missing doc comments and add unused annotations to the derivs

6291ff3

parameters

Declare new header files to CMake

c176354

Increase timeout

d70785d

Modify 3D error bound computation

11b2f8e

tpadioleau force-pushed the multid_splines branch from 2a01541 to 11b2f8e Compare October 10, 2025 05:22

tpadioleau previously approved these changes Oct 10, 2025

View reviewed changes

Slightly increase tolerance

75cc670

tpadioleau dismissed their stale review via 75cc670 October 10, 2025 07:22

tpadioleau merged commit deae66e into CExA-project:main Oct 10, 2025
82 of 83 checks passed

tretre91 deleted the multid_splines branch November 3, 2025 14:36


		LowerExtrapolationRule3 m_lower_extrap_rule_3;

		UpperExtrapolationRule3 m_upper_extrap_rule_3;

		template <class Layout, class BatchedInterpolationDDom>
		void operator()(

Add 3D Splines (with periodic or greville boundary conditions) #925

Add 3D Splines (with periodic or greville boundary conditions) #925

Uh oh!

Conversation

tretre91 commented Sep 30, 2025

Uh oh!

codecov bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

science-enthusiast Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tpadioleau commented Oct 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 30, 2025 •

edited

Loading

science-enthusiast Oct 8, 2025 •

edited

Loading