move nan defaulting out of ndarray to fielddata,tfdata

[PhilipDeegan]

configured in ndarrayvector is a bit heavy handed

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors default value initialization patterns across core data structures by introducing NaN constants for field/tensor initialization, replacing direct value parameters with std::optional in Grid and NdArrayVector constructors, and adding public type aliases to standardize value_type access across the hierarchy.

Changes

Cohort / File(s)	Summary
Field and TensorField Data Structures src/amr/data/field/field_data.hpp, src/amr/data/tensorfield/tensor_field_data.hpp	Added private NaN constant using std::numeric_limits<value_type>::quiet_NaN(), introduced public value_type type alias in TensorFieldData, updated constructors to pass NaN parameter to Grid initialization, and adjusted header includes to add <limits> and reorganize dependencies.
Core Grid Constructor Refactoring src/core/data/grid/grid.hpp	Restructured Grid constructors to use std::optional<value_type> instead of direct value parameters; replaced multiple overloaded constructors with simplified variants accepting either fixed-size dims array or variadic integral dimensions; added template constructor with integration and static_assert for dimension validation.
NdArray Vector Initialization src/core/data/ndarray/ndarray_vector.hpp	Simplified NdArrayVector constructor to single overload accepting std::optional<DataType> value parameter; introduced private init_optional helper for optional-based data initialization; added public value_type type alias; removed previous variadic and multiple overload paths.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix master improved #1083: Modifies the same core field/grid/tensorfield headers with coordinated changes to default initialization, NaN handling, and tensor-field support patterns.

Suggested reviewers

• nicolasaunai

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No description was provided by the author, making it impossible to assess whether the description relates to the changeset.	Add a pull request description that explains the motivation, approach, and impact of moving NaN defaulting logic from ndarray to fielddata and tfdata.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main objective: refactoring NaN defaulting logic from NdArrayVector to FieldData and TensorFieldData, which aligns with the substantial changes across multiple files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/amr/data/field/field_data.hpp (1)

36-38: Non-canonical declaration specifier order.

auto constexpr static is valid but unconventional. The idiomatic C++ order is static constexpr auto. Same applies to the identical declaration in tensor_field_data.hpp (line 44).

Suggested fix

-        using SetEqualOp          = core::Equals<value_type>;
-        auto constexpr static NaN = std::numeric_limits<value_type>::quiet_NaN();
+        using SetEqualOp                    = core::Equals<value_type>;
+        static constexpr auto NaN = std::numeric_limits<value_type>::quiet_NaN();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/amr/data/field/field_data.hpp` around lines 36 - 38, Change the
non-idiomatic declaration order for the NaN constant: replace the declaration of
NaN (currently declared as "auto constexpr static NaN") with the canonical
"static constexpr auto NaN" in field_data.hpp, and make the identical change to
the matching NaN declaration in tensor_field_data.hpp (the same symbol name NaN)
so both use the conventional static constexpr order.

src/core/data/grid/grid.hpp (1)

58-64: Dimension check could be unified into the requires clause.

The requires clause checks that all Dims are integral, while static_assert checks the count. Merging both into requires would reject mismatched calls at overload resolution rather than inside the body, yielding clearer diagnostics at call sites.

Suggested consolidation

     template<typename... Dims>
     Grid(std::string const& name, PhysicalQuantity qty, Dims... dims)
-        requires(std::is_integral_v<Dims> && ...)
+        requires(sizeof...(Dims) == dimension && (std::is_integral_v<Dims> && ...))
         : Grid{name, qty, std::array{dims...}}
     {
-        static_assert(sizeof...(Dims) == dimension, "Invalid dimension");
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/data/grid/grid.hpp` around lines 58 - 64, Update the template
constructor Grid(std::string const& name, PhysicalQuantity qty, Dims... dims) so
the arity check is part of the requires expression: replace the current
requires(std::is_integral_v<Dims> && ...) and remove the in-body
static_assert(sizeof...(Dims) == dimension); instead use a combined requires
such as requires((std::is_integral_v<Dims> && ...) && (sizeof...(Dims) ==
dimension)) so overload resolution rejects calls with wrong count or
non-integral Dims; keep the delegating initializer : Grid{name, qty,
std::array{dims...}} unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/amr/data/field/field_data.hpp`:
- Around line 36-38: Change the non-idiomatic declaration order for the NaN
constant: replace the declaration of NaN (currently declared as "auto constexpr
static NaN") with the canonical "static constexpr auto NaN" in field_data.hpp,
and make the identical change to the matching NaN declaration in
tensor_field_data.hpp (the same symbol name NaN) so both use the conventional
static constexpr order.

In `@src/core/data/grid/grid.hpp`:
- Around line 58-64: Update the template constructor Grid(std::string const&
name, PhysicalQuantity qty, Dims... dims) so the arity check is part of the
requires expression: replace the current requires(std::is_integral_v<Dims> &&
...) and remove the in-body static_assert(sizeof...(Dims) == dimension); instead
use a combined requires such as requires((std::is_integral_v<Dims> && ...) &&
(sizeof...(Dims) == dimension)) so overload resolution rejects calls with wrong
count or non-integral Dims; keep the delegating initializer : Grid{name, qty,
std::array{dims...}} unchanged.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7d962d and e00fff6.

📒 Files selected for processing (4)

• src/amr/data/field/field_data.hpp
• src/amr/data/tensorfield/tensor_field_data.hpp
• src/core/data/grid/grid.hpp
• src/core/data/ndarray/ndarray_vector.hpp