Skip to content

allow arbitrary vector allocators for restarts#1161

Open
PhilipDeegan wants to merge 1 commit intoPHAREHUB:masterfrom
PhilipDeegan:putgetrestart
Open

allow arbitrary vector allocators for restarts#1161
PhilipDeegan wants to merge 1 commit intoPHAREHUB:masterfrom
PhilipDeegan:putgetrestart

Conversation

@PhilipDeegan
Copy link
Copy Markdown
Member

@PhilipDeegan PhilipDeegan commented Feb 26, 2026

samrai restart putVector style functions only support vectors with the default standard allocator

we could ask samrai folks to allow for other allocators, but that might never even happen

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0cf1dbcb-db0a-4b11-8492-97ff81293a61

📥 Commits

Reviewing files that changed from the base of the PR and between 1b388e3 and dbf1d35.

📒 Files selected for processing (6)
  • src/amr/data/field/field_data.hpp
  • src/amr/data/particles/particles_data.hpp
  • src/amr/data/tensorfield/tensor_field_data.hpp
  • src/amr/samrai.cpp
  • src/amr/samrai.hpp
  • src/core/utilities/types.hpp
✅ Files skipped from review due to trivial changes (3)
  • src/core/utilities/types.hpp
  • src/amr/data/field/field_data.hpp
  • src/amr/samrai.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/amr/data/tensorfield/tensor_field_data.hpp
  • src/amr/data/particles/particles_data.hpp
  • src/amr/samrai.cpp

📝 Walkthrough

Walkthrough

Refactors restart I/O to use new helper templates, adds those helpers in amr/samrai.hpp, updates SAMRAI lifecycle initialization/teardown in SamraiLifeCycle, and adds a SFINAE-friendly dependent_false_v utility. No public API signatures changed. (49 words)

Changes

Cohort / File(s) Summary
SAMRAI helpers
src/amr/samrai.hpp
Adds getVectorFromRestart and putVectorToRestart template helpers (supporting double and int) and adjusts includes; centralizes vector restart I/O.
AMR data restart I/O
src/amr/data/field/field_data.hpp, src/amr/data/particles/particles_data.hpp, src/amr/data/tensorfield/tensor_field_data.hpp
Replaces direct restart_db->getDoubleArray/getVector and putVector calls with the new helper functions; minor const-correctness and include tweaks.
SAMRAI lifecycle
src/amr/samrai.cpp
Explicit SAMRAI/MPI initialization and shutdown moved into SamraiLifeCycle (init/startup/shutdown/finalize); adds logger warning appender and conditional PHLOP timing setup; modifies restart-manager reset behavior.
Utilities
src/core/utilities/types.hpp
Adds template<typename> inline constexpr bool dependent_false_v = false; for use in SFINAE/compile-time checks.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant LifeCycle as SamraiLifeCycle
participant MPI as SAMRAI_MPI
participant Manager as SAMRAIManager
participant Restart as RestartManager
participant PHLOP as phlop::ScopeTimerMan

LifeCycle->>MPI: init()
LifeCycle->>Manager: initialize()
LifeCycle->>Manager: startup()
LifeCycle->>Restart: getRestartManager() (used later)
alt during reset
    LifeCycle->>Manager: shutdown()
    LifeCycle->>Restart: clearRestartItems()
    LifeCycle->>Manager: startup()
end
LifeCycle->>Manager: shutdown() (destructor)
LifeCycle->>Manager: finalize()
LifeCycle->>MPI: finalize()
alt PHLOP enabled
    LifeCycle->>PHLOP: ScopeTimerMan::reset()
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

experimental

Suggested reviewers

  • UCaromel
  • nicolasaunai
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main objective of the changeset: enabling support for arbitrary vector allocators in restart operations.
Description check ✅ Passed The description relates to the changeset by explaining the motivation: SAMRAI's restart functions only support default allocators, and this PR works around that limitation.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/amr/samrai.hpp (1)

72-83: Consider making empty vectors a no-op in the write helper.

Adding an early vec.empty() return here would centralize behavior and avoid repeating zero-size guards at call sites.

♻️ Proposed refactor
 template<typename T, typename A>
 void putVectorToRestart(auto& db, auto const& path, std::vector<T, A> const& vec)
 {
+    if (vec.empty())
+        return;
+
     if constexpr (std::is_same_v<T, double>)
         db.putDoubleArray(path, vec.data(), vec.size());

     else if constexpr (std::is_same_v<T, int>)
         db.putIntegerArray(path, vec.data(), vec.size());
As per coding guidelines, `**/*.hpp`: “Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/amr/samrai.hpp` around lines 72 - 83, The helper putVectorToRestart
should early-return on empty vectors to centralize behavior and avoid callers
duplicating zero-size checks; modify putVectorToRestart to check if vec.empty()
and return immediately before calling db.putDoubleArray or db.putIntegerArray,
preserving the existing type-specific branches (std::is_same_v<T,double> and
std::is_same_v<T,int>) and keep the static_assert for unsupported types.
🤖 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/samrai.hpp`:
- Around line 72-83: The helper putVectorToRestart should early-return on empty
vectors to centralize behavior and avoid callers duplicating zero-size checks;
modify putVectorToRestart to check if vec.empty() and return immediately before
calling db.putDoubleArray or db.putIntegerArray, preserving the existing
type-specific branches (std::is_same_v<T,double> and std::is_same_v<T,int>) and
keep the static_assert for unsupported types.

ℹ️ 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 1b388e3.

📒 Files selected for processing (6)
  • src/amr/data/field/field_data.hpp
  • src/amr/data/particles/particles_data.hpp
  • src/amr/data/tensorfield/tensor_field_data.hpp
  • src/amr/samrai.cpp
  • src/amr/samrai.hpp
  • src/core/utilities/types.hpp

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