Add spline compatibility check helper #878

yasahi-hpc · 2025-06-23T09:14:24Z

This PR aims at a helper to check the consistency of spline builder and evaluator.
For some reason, I need double parentheses to use ASSERT_FALSE like
ASSERT_FALSE((ddc::is_spline_compatible_v<Builder2D_1, Builder2D_1>));

codecov · 2025-06-23T09:31:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.94%. Comparing base (e5a1bf8) to head (67967c7).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #878   +/-   ##
=======================================
  Coverage   89.94%   89.94%           
=======================================
  Files          55       55           
  Lines        2863     2863           
  Branches      976      976           
=======================================
  Hits         2575     2575           
  Misses         91       91           
  Partials      197      197

☔ 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.

yasahi-hpc · 2025-06-23T11:03:41Z

@tpadioleau

Following changes are needed?

Only expose is_<check>_v
Add docs to is_<check>_v
Use parentheses to &&?

template <class Builder, class Evaluator>
 struct is_spline_compatible<Builder, Evaluator,
        std::enable_if_t<is_spline_builder_v<Builder> && is_spline_evaluator_v<Evaluator>
                         || is_spline_builder_v<Evaluator> && is_spline_evaluator_v<Builder> >>

Should be

template <class Builder, class Evaluator>
 struct is_spline_compatible<Builder, Evaluator,
        std::enable_if_t<(is_spline_builder_v<Builder> && is_spline_evaluator_v<Evaluator>)
                         || (is_spline_builder_v<Evaluator> && is_spline_evaluator_v<Builder>) >>

tpadioleau · 2025-06-23T11:52:05Z

@tpadioleau

Following changes are needed?

Only expose is_<check>_v

You can keep is_<check> in the current namespace as the other traits. We can put them in a detail namespace later.

Add docs to is_<check>_v

Yes please

Use parentheses to &&?

Yes I guess the point of the linter is to clarify the intent.

yasahi-hpc · 2025-06-23T11:59:27Z

@tpadioleau
Following changes are needed?

Only expose is_<check>_v

You can keep is_<check> in the current namespace as the other traits. We can put them in a detail namespace later.

OK

Add docs to is_<check>_v

Yes please

What kind of docstrings do you prefer?
I could not see docs for these constants.
is_chunk_v

Use parentheses to &&?

Yes I guess the point of the linter is to clarify the intent.

OK

EmilyBourne · 2025-06-23T12:11:20Z

include/ddc/kernels/splines/spline_traits.hpp

+};
+
+template <class Builder, class Evaluator>
+struct is_spline_compatible<Builder, Evaluator,


I'm not sure this name is very explicit. I would expect it to check if a spline is compatible with a builder or if a spline is compatible with an evaluator, not if a spline builder is compatible with an evaluator

I'm not sure what to suggest instead though

Suggested change

struct is_spline_compatible<Builder, Evaluator,

struct is_spline_interpolation_possible<Builder, Evaluator,

Suggested change

struct is_spline_compatible<Builder, Evaluator,

struct is_interpolation_possible<Builder, Evaluator,

?

I do not have a strong preference.
But, I would prefer to include a term spline at least and ideally not too long

I definitely agree that it should not be too long.

I'm not sure it is important to include spline though? If you had a different intermediate representation it is likely that you would still need a similar check for a compatible Builder and Evaluator. That is why I suggested is_interpolation_possible

If splines are important then maybe shares_spline_basis? But this doesn't cover the Memory/Execution space aspects

After some discussion, we decided to rename it as is_evaluator_admissible<Builder, Evaluator> and we do not accept is_evaluator_admissible<Evaluator, Builder>

yasahi-hpc · 2025-06-23T14:22:09Z

@tpadioleau Can you format this
clangformat 20 is unavailable on my env

tpadioleau · 2025-06-23T14:30:40Z

@tpadioleau Can you format this clangformat 20 is unavailable on my env

Done

tpadioleau

Thanks for the contribution! Just a few comments about the form. Can you also add your name to the authors ?

tpadioleau · 2025-06-23T16:37:21Z

include/ddc/kernels/splines/spline_traits.hpp

+#include "spline_evaluator_2d.hpp"
+
+namespace ddc {
+template <class T>


Suggested change

template <class T>

template <class T>

tpadioleau · 2025-06-23T16:37:50Z

include/ddc/kernels/splines/spline_traits.hpp

+#include <cassert>
+#include <cstddef>
+#include <memory>
+#include <optional>
+#include <stdexcept>
+#include <tuple>
+#include <type_traits>
+#include <utility>


Can you update the list of headers ?

tpadioleau · 2025-06-23T16:41:40Z

include/ddc/kernels/splines/spline_traits.hpp

+inline constexpr bool is_spline_builder_v = is_spline_builder<T>::value;
+
+template <class T>
+struct is_spline_builder2D : std::false_type


Suggested change

struct is_spline_builder2D : std::false_type

struct is_spline_builder2d : std::false_type

tpadioleau · 2025-06-23T16:41:50Z

include/ddc/kernels/splines/spline_traits.hpp

+        ddc::BoundCond BcLower2,
+        ddc::BoundCond BcUpper2,
+        ddc::SplineSolver Solver>
+struct is_spline_builder2D<SplineBuilder2D<


Suggested change

struct is_spline_builder2D<SplineBuilder2D<

struct is_spline_builder2d<SplineBuilder2D<

tpadioleau · 2025-06-23T16:41:56Z

include/ddc/kernels/splines/spline_traits.hpp

+ *  @tparam T The type to be checked if is a SplineBuilder2D
+ */
+template <class T>
+inline constexpr bool is_spline_builder2D_v = is_spline_builder2D<T>::value;


Suggested change

inline constexpr bool is_spline_builder2D_v = is_spline_builder2D<T>::value;

inline constexpr bool is_spline_builder2d_v = is_spline_builder2d<T>::value;

tpadioleau · 2025-06-23T16:42:25Z

include/ddc/kernels/splines/spline_traits.hpp

+inline constexpr bool is_spline_evaluator_v = is_spline_evaluator<T>::value;
+
+template <class T>
+struct is_spline_evaluator2D : std::false_type


Suggested change

struct is_spline_evaluator2D : std::false_type

struct is_spline_evaluator2d : std::false_type

tpadioleau · 2025-06-23T16:42:32Z

include/ddc/kernels/splines/spline_traits.hpp

+        class UpperExtrapolationRule1,
+        class LowerExtrapolationRule2,
+        class UpperExtrapolationRule2>
+struct is_spline_evaluator2D<SplineEvaluator2D<


Suggested change

struct is_spline_evaluator2D<SplineEvaluator2D<

struct is_spline_evaluator2d<SplineEvaluator2D<

tpadioleau · 2025-06-23T16:42:39Z

include/ddc/kernels/splines/spline_traits.hpp

+ *  @tparam T The type to be checked if is a SplineEvaluator2D
+ */
+template <class T>
+inline constexpr bool is_spline_evaluator2D_v = is_spline_evaluator2D<T>::value;


Suggested change

inline constexpr bool is_spline_evaluator2D_v = is_spline_evaluator2D<T>::value;

inline constexpr bool is_spline_evaluator2d_v = is_spline_evaluator2D<T>::value;

tpadioleau · 2025-06-23T16:43:52Z

tests/splines/spline_traits.cpp

+#include <cstddef>
+#include <stdexcept>
+#include <vector>


Can you update the list of headers ?

tpadioleau · 2025-06-23T16:48:54Z

tests/splines/spline_traits.cpp

+
+#include "test_utils.hpp"
+
+struct DimX


Can you put struct DimX, DDimX, DimY, DDimY and BSplinesTraits in an inline namespace ?

inline namespace anonymous_namespace_workaround_spline_traits_cpp { }

EmilyBourne reviewed Jun 23, 2025

View reviewed changes

yasahi-hpc added 2 commits June 23, 2025 16:20

Add spline compatibility check helper

e4dc899

rename and add docstrings

b33f82c

yasahi-hpc force-pushed the check-compatibility-of-splines branch from 64b4312 to b33f82c Compare June 23, 2025 14:20

yasahi-hpc and others added 2 commits June 23, 2025 16:28

Fix: typo

57df408

Format

67967c7

yasahi-hpc requested a review from tpadioleau June 23, 2025 16:09

tpadioleau requested changes Jun 23, 2025

View reviewed changes

	struct is_spline_compatible<Builder, Evaluator,
	struct is_spline_interpolation_possible<Builder, Evaluator,

	struct is_spline_compatible<Builder, Evaluator,
	struct is_interpolation_possible<Builder, Evaluator,

	struct is_spline_builder2D : std::false_type
	struct is_spline_builder2d : std::false_type

	struct is_spline_builder2D<SplineBuilder2D<
	struct is_spline_builder2d<SplineBuilder2D<

	inline constexpr bool is_spline_builder2D_v = is_spline_builder2D<T>::value;
	inline constexpr bool is_spline_builder2d_v = is_spline_builder2d<T>::value;

	struct is_spline_evaluator2D : std::false_type
	struct is_spline_evaluator2d : std::false_type

	struct is_spline_evaluator2D<SplineEvaluator2D<
	struct is_spline_evaluator2d<SplineEvaluator2D<

	inline constexpr bool is_spline_evaluator2D_v = is_spline_evaluator2D<T>::value;
	inline constexpr bool is_spline_evaluator2d_v = is_spline_evaluator2D<T>::value;

Add spline compatibility check helper #878

Are you sure you want to change the base?

Add spline compatibility check helper #878

Conversation

yasahi-hpc commented Jun 23, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

yasahi-hpc commented Jun 23, 2025

Uh oh!

tpadioleau commented Jun 23, 2025

Uh oh!

yasahi-hpc commented Jun 23, 2025

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!

yasahi-hpc commented Jun 23, 2025

Uh oh!

tpadioleau commented Jun 23, 2025

Uh oh!

tpadioleau left a comment

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!

Uh oh!

codecov bot commented Jun 23, 2025 •

edited

Loading