check for domain overlaps #1129
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded same-level neighbor utilities and domain-overlap checks; applied a const qualifier to a level parameter; wrapped hybrid level initialization overlap validation in try/catch and integrated MPI-aware error throwing; extended DictionaryException with an id() accessor and updated Simulator to catch DictionaryException and trigger emergency dumps across all levels when matching configured IDs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
Participant Simulator
Participant HybridInitializer
Participant AMRUtils
Participant DictionaryException
Participant DumpManager
Participant Logger
User->>Simulator: start initialize()
Simulator->>HybridInitializer: initialize(hierarchy, levelNumber)
HybridInitializer->>AMRUtils: noDomainOverlapsOn(hierarchy, levelNumber)
AMRUtils-->>HybridInitializer: throws DictionaryException (on overlap) / returns OK
alt overlap/exception
HybridInitializer->>Simulator: propagate exception
Simulator->>DictionaryException: catch
Simulator->>DumpManager: handle_dictionary_exception -> dump_level (all levels)
DumpManager->>Logger: log dumps
else no overlap
HybridInitializer-->>Simulator: initialization complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
PhilipDeegan
force-pushed
the
catastroph
branch
3 times, most recently
from
2f6d1eb to
099c89d
Compare
PhilipDeegan
force-pushed
the
catastroph
branch
from
099c89d to
2a44220
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/simulator/test_exceptions.py`:
- Line 101: The test currently calls Run(out).GetN(0, "protons").plot(), which
can fail in headless CI; either remove the .plot() call and assert the data
exists by calling Run(out).GetN(0, "protons") (or inspecting its contents), or
set a non-interactive matplotlib backend (e.g., call matplotlib.use('Agg') at
the top of the test module before any plotting imports) to ensure .plot() won't
require a display; update the test to use one of these approaches and keep the
check focused on GetN.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/resources_manager/amr_utils.hppsrc/core/errors.hppsrc/simulator/simulator.hpptests/simulator/test_exceptions.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/errors.hpp
- src/simulator/simulator.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/resources_manager/amr_utils.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-11-10T09:37:57.021Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1084
File: src/diagnostic/diagnostic_model_view.hpp:219-221
Timestamp: 2025-11-10T09:37:57.021Z
Learning: In PHARE, temporary fields like tmpField_, tmpVec_, and tmpTensor_ in src/diagnostic/diagnostic_model_view.hpp use a name-based resource allocation pattern. Fields with names "PHARE_sumField", "PHARE_sumVec", and "PHARE_sumTensor" are allocated globally elsewhere in the resources manager (e.g., in src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp), and components reference the same storage through identically-named field objects. These do not require local allocation via rm.enumerate().
Applied to files:
src/amr/level_initializer/hybrid_level_initializer.hpp
🧬 Code graph analysis (2)
src/amr/level_initializer/hybrid_level_initializer.hpp (2)
src/amr/resources_manager/amr_utils.hpp (4)
noDomainOverlapsOn(210-218)noDomainOverlapsOn(210-210)noDomainOverlapsOn(220-224)noDomainOverlapsOn(220-220)src/core/utilities/types.hpp (4)
any(359-363)any(359-360)any(382-385)any(382-382)
tests/simulator/test_exceptions.py (3)
pyphare/pyphare/simulator/simulator.py (1)
run(200-238)pyphare/pyphare/pharesee/run/run.py (2)
Run(35-289)GetN(120-122)pyphare/pyphare/pharein/diagnostics.py (1)
FluidDiagnostics(292-304)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (12)
tests/simulator/test_exceptions.py (3)
6-6: LGTM!Import correctly added for the
Runclass used later in the test.
64-67: LGTM!The FluidDiagnostics with empty
write_timestampsis correctly set up to enable emergency dumps without writing during normal execution. The comment clarifies the intent.
80-80: LGTM!The
allow_emergency_dumpsoption correctly enables the emergency dump feature that this test validates.src/amr/resources_manager/amr_utils.hpp (5)
19-19: LGTM!The
<stdexcept>include is correctly added to support thestd::runtime_errorusage innoDomainOverlapsOn.
200-208: LGTM!Clean utility function that encapsulates the same-level neighbor query. The
NO_DISCARDattribute is appropriate since discarding the result would indicate a logic error.
210-224: LGTM!The domain overlap detection logic is well-structured:
- Clear error message identifies the problematic level
- The check is O(patches × neighbors) which is acceptable for validation
- The overloaded version for all levels correctly delegates to the single-level version
238-238: LGTM!Good refactoring to use the new
getSameLevelNeighborsutility, eliminating code duplication.
274-275: LGTM!Minor style improvement using pre-increment, which is idiomatic for loop counters in C++.
src/amr/level_initializer/hybrid_level_initializer.hpp (4)
4-20: LGTM!Include reorganization is clean and properly structured:
- Core headers grouped together
- AMR headers grouped together
<exception>header added forstd::exceptionusage in catch block
43-43: LGTM!Adding
constto the parameter clarifies intent and prevents accidental modification.
185-185: No concerns.Formatting change only.
86-96: The exception handling is correct; no changes needed.
PHARE_LOG_ERRORautomatically registers the error withcore::Errors::instance()by calling itslog()method, which adds the error to the internal map. The subsequentcore::mpi::any(core::Errors::instance().any())check will correctly detect this logged error and throwDictionaryException. The exception is not swallowed.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
No description provided.