Skip to content

fix: emergency dump one full-hierarchy step, not per-level - #1276

Draft
igirault wants to merge 1 commit into
PHAREHUB:masterfrom
igirault:fix-emergency-dump-amr
Draft

fix: emergency dump one full-hierarchy step, not per-level#1276
igirault wants to merge 1 commit into
PHAREHUB:masterfrom
igirault:fix-emergency-dump-amr

Conversation

@igirault

@igirault igirault commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fix by claude for #1275. Here his explanation:

Summary

  • Emergency dumps (handle_dictionary_exception) looped dump_level(ilvl) per AMR level. Each dump_level call bumps VTKHDF's /Steps/NSteps and appends a Values entry once while writing only that level's data — VTKHDF OverlappingAMR requires one step to hold all levels, so a multi-level emergency dump produced split, partial steps that ParaView renders as blank. Single-level/no-AMR runs happened to survive since dump_level(0) is effectively a full snapshot in that case.
  • dump_level also skipped compute(), leaving derived quantities ( P, V) stale in the emergency snapshot.
  • Adds IDiagnosticsManager::dump_all(timeStamp): runs compute() on every diagnostic, then a single full-hierarchy dump() — one consistent VTKHDF step with all levels. The emergency handler now calls dump_all instead of the per-level loop.

Test plan

  • Existing emergency-dump exercising tests still build/pass (verified on the branch this fix was developed on)
  • CI

🤖 Generated with Claude Code

handle_dictionary_exception looped dump_level(ilvl) per level. Each
dump_level bumps VTKHDF /Steps/NSteps and appends Values once while
writing only that level's data. VTKHDF OverlappingAMR requires one step
to hold all levels, so multi-level emergency dumps produced split,
partial steps that ParaView renders as blank (single-level/no-AMR runs
survived because dump_level(0) is effectively a full snapshot).
dump_level also skipped compute(), leaving B=B1+B0/P/V/Etot stale.

Add IDiagnosticsManager::dump_all(timeStamp): compute() every diagnostic
then one full-hierarchy dump() -> a single consistent step with all
levels. Emergency handler calls dump_all instead of the per-level loop.

Fixes PHAREHUB#1275

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The diagnostics API adds an unconditional full-hierarchy dump operation. The concrete manager gathers all diagnostics into one writer call, and simulator emergency handling uses it instead of dumping each AMR level separately.

Changes

Emergency dump flow

Layer / File(s) Summary
Full-hierarchy dump contract and implementation
src/diagnostic/diagnostic_manager.hpp, src/diagnostic/diagnostics.hpp
Adds dump_all, recomputes derived diagnostics, writes the full active set in one operation, and updates the null implementation.
Emergency handler integration
src/simulator/simulator.hpp
Replaces per-level emergency dumps with a single full-hierarchy dump at the current simulation time.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Simulator
  participant DiagnosticsManager
  participant DiagnosticWriter
  Simulator->>DiagnosticsManager: dump_all(currentTime_)
  DiagnosticsManager->>DiagnosticsManager: recompute derived quantities
  DiagnosticsManager->>DiagnosticWriter: dump all active diagnostics
Loading

Possibly related PRs

  • PHAREHUB/PHARE#1123: Modifies the emergency dictionary-exception path to trigger an all-level diagnostics dump.

Suggested reviewers: philipdeegan

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR directly addresses issue #1275 by fixing VTKHDF emergency dumps with multiple AMR levels to be visualizable in ParaView.
Out of Scope Changes check ✅ Passed All changes are scoped to the emergency dump fix: new dump_all method, NullOpDiagnosticsManager override, and emergency handler update.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly summarizes the main change: switching emergency dumps from per-level output to a single full-hierarchy step.
Description check ✅ Passed The description matches the implemented fix and explains the emergency dump behavior change, compute step, and test plan.
✨ 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.

@igirault
igirault marked this pull request as draft July 23, 2026 13:44
@igirault
igirault requested a review from PhilipDeegan July 23, 2026 13:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/simulator/simulator.hpp (1)

591-595: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Add a regression test for the emergency-dump contract.

Exercise a hierarchy with at least three AMR levels and assert that the emergency path performs exactly one full-hierarchy dump(...) after recomputing all diagnostics. This directly protects issue #1275 from regressing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/simulator/simulator.hpp` around lines 591 - 595, Add a regression test
for the emergency-dump path around the simulator code invoking
dMan->dump_all(currentTime_). Build a hierarchy with at least three AMR levels,
trigger the emergency snapshot, and assert diagnostics are recomputed before
exactly one full-hierarchy dump occurs, rather than one dump per level. Use the
existing dump and diagnostics test hooks or mocks to verify the call count and
ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/simulator/simulator.hpp`:
- Around line 591-595: Add a regression test for the emergency-dump path around
the simulator code invoking dMan->dump_all(currentTime_). Build a hierarchy with
at least three AMR levels, trigger the emergency snapshot, and assert
diagnostics are recomputed before exactly one full-hierarchy dump occurs, rather
than one dump per level. Use the existing dump and diagnostics test hooks or
mocks to verify the call count and ordering.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8526c185-97fc-4f39-9345-737570da0ccd

📥 Commits

Reviewing files that changed from the base of the PR and between 6ead776 and 99895c4.

📒 Files selected for processing (3)
  • src/diagnostic/diagnostic_manager.hpp
  • src/diagnostic/diagnostics.hpp
  • src/simulator/simulator.hpp

@nicolasaunai
nicolasaunai requested a review from UCaromel July 23, 2026 21:09
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.

1 participant