overlap Difference hierarchies#1115
Conversation
📝 WalkthroughWalkthroughAdds operator-based field transactions and ion max-border handling across messenger/refiner/transaction layers; introduces a SetMax utility, Python DataSelector/select helpers, hierarchy utilities, an MPI diagnostics script, and multiple small Python/C++ guards and metadata updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/amr/messengers/field_sum_transaction.hpp (1)
6-11: Minor include organization note.The includes could be reorganized to follow standard convention: local includes (quoted) grouped first, then system includes (angle brackets) grouped after. Currently they're interleaved with blank lines. This is a minor style point and not critical, but worth noting for consistency across the codebase.
pyphare/pyphare/core/box.py (2)
186-187: Consider clearer variable naming.Static analysis flags the variable name
las ambiguous (could be confused with1). Consider usinglowerandupperfor clarity.Apply this diff:
def select(data, box): - return data[tuple([slice(l, u + 1) for l, u in zip(box.lower, box.upper)])] + return data[tuple([slice(lower, upper + 1) for lower, upper in zip(box.lower, box.upper)])]Note: Static analysis also suggests adding
strict=Truetozip()(Python 3.10+), but sinceBox.__init__enforces equal-length arrays, this is optional.
189-203: LGTM! DataSelector enables Box-based slicing.The implementation correctly enables syntax like
DataSelector(data)[box] = valby returning views that support slice assignment. The dual support for Box and regular slicing provides good flexibility.The docstring could be more descriptive:
class DataSelector: """ - can't assign return to function unless [] - usage - DataSelector(data)[box] = val + Wrapper enabling Box-based slicing and assignment on NumPy arrays. + + Usage: + DataSelector(data)[box] = val # Assign to Box region + view = DataSelector(data)[box] # Get Box region """src/core/data/field/field_box.hpp (1)
71-82: Consider reusingoperate_on_fieldswithSetMaxoperator to avoid duplication.This function duplicates the iteration logic from
operate_on_fieldsabove. SinceSetMaxalready exists incore/utilities/types.hppand is used elsewhere (e.g.,field_data.hpp), consider:-void max_of_fields(auto& dst, auto const& src) -{ - assert(dst.lcl_box.size() == src.lcl_box.size()); - auto src_it = src.lcl_box.begin(); - auto dst_it = dst.lcl_box.begin(); - for (; dst_it != dst.lcl_box.end(); ++src_it, ++dst_it) - { - auto& dst_val = dst.field(*dst_it); - auto& src_val = src.field(*src_it); - dst_val = std::max(dst_val, src_val); - } -} +void max_of_fields(auto& dst, auto const& src) +{ + using value_type = std::decay_t<typename std::decay_t<decltype(dst.field)>::value_type>; + operate_on_fields<SetMax<value_type>>(dst, src); +}Also, if keeping the current implementation, line 79 should use
auto const& src_valsincesrcis const-qualified.tests/functional/harris/harris_2d_diff.py (3)
68-69: Rename ambiguous variablelfor clarity.The variable
lis easily confused with1orI. Consider using a more descriptive name likelengthorscale.- def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) + def S(y, y0, length): + return 0.5 * (1.0 + np.tanh((y - y0) / length))
183-190: Remove extraneousfprefix from strings without placeholders.Several f-strings have no interpolation, as flagged by static analysis:
ranks.plot( - filename=plot_file_for_qty(plot_dir, f"ranks", new_time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", new_time, f"L{ilvl}"),Similar changes needed at lines 215 (
f"ionCharge"→"ionCharge") and 230 (f"ionMass"→"ionMass").
276-282: Usenot ininstead ofnot ... infor membership test.More idiomatic Python style:
- if not "Unable to synchronously open object" in err: # no diag for time + if "Unable to synchronously open object" not in err: # no diag for timesrc/amr/messengers/refiner.hpp (1)
116-137: Minor: Clarify comment wording.The comments state "schedule used to == max of density and flux" - the
==notation is slightly confusing. Consider rewording to "schedule used to compute max of..." for consistency with the sum variant comment style ("schedule used to += density and flux").- // schedule used to == max of density and flux for populations + // schedule used to compute max of density and flux for populations // on complete overlaped ghost box nodes else if constexpr (Type == RefinerType::PatchFieldBorderMax) { @@ -128,7 +128,7 @@ } - // schedule used to == max of density and flux for populations + // schedule used to compute max of density and flux for populations // on complete overlaped ghost box nodes else if constexpr (Type == RefinerType::PatchVecFieldBorderMax) {src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
490-502: Hardcoded size assertions may be fragile.The assertions
ionFluxBorderMaxRefiners_.size() == 1andionDensityBorderMaxRefiners_.size() == 2are hardcoded magic numbers. If the configuration inregisterInitCommschanges (e.g.,maxBorderFieldsormaxBorderVecFieldssizes change), these assertions will fail without clear context.Consider either:
- Deriving the expected size from a shared constant or configuration
- Adding a clarifying comment explaining why these specific sizes are expected
src/amr/messengers/field_max_transaction.hpp (1)
17-18: Incomplete documentation.The class documentation contains a TODO placeholder. Consider completing the description to explain the purpose of
FieldBorderMaxTransaction(e.g., "handles max-reduction operations for field border synchronization across patches").-/** * @brief FieldBorderMaxTransaction is TODO +/** * @brief FieldBorderMaxTransaction handles max-reduction operations for field border + * synchronization during refinement across SAMRAI patches. * */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
pyphare/pyphare/core/box.py(1 hunks)pyphare/pyphare/core/gridlayout.py(3 hunks)pyphare/pyphare/pharesee/geometry.py(3 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/patchdata.py(1 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)pyphare/pyphare/simulator/simulator.py(2 hunks)src/amr/data/field/field_data.hpp(8 hunks)src/amr/data/tensorfield/tensor_field_data.hpp(2 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(1 hunks)src/amr/messengers/field_max_transaction.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(7 hunks)src/amr/messengers/hybrid_messenger.hpp(1 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/refiner.hpp(6 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(1 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/mpi_utils.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)tests/functional/harris/harris_2d_diff.py(1 hunks)
🧰 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/core/utilities/types.hppsrc/amr/messengers/hybrid_messenger_strategy.hppsrc/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/messengers/mhd_hybrid_messenger_strategy.hppsrc/amr/messengers/hybrid_messenger_info.hppsrc/amr/messengers/hybrid_messenger.hppsrc/core/data/field/field_box.hppsrc/amr/messengers/refiner.hppsrc/amr/physical_models/hybrid_model.hppsrc/core/data/tensorfield/tensorfield.hppsrc/amr/messengers/field_sum_transaction.hppsrc/amr/data/tensorfield/tensor_field_data.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/core/utilities/mpi_utils.hppsrc/amr/data/field/field_data.hppsrc/amr/messengers/field_max_transaction.hpp
🧠 Learnings (5)
📓 Common learnings
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: 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: 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: 2024-10-18T13:23:32.074Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.pypyphare/pyphare/pharesee/hierarchy/hierarchy.py
📚 Learning: 2025-02-06T10:52:56.461Z
Learnt from: rochSmets
Repo: PHAREHUB/PHARE PR: 921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
Applied to files:
src/amr/level_initializer/hybrid_level_initializer.hpp
📚 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/messengers/refiner.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/field_max_transaction.hpp
📚 Learning: 2024-10-22T10:05:54.014Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 0
File: :0-0
Timestamp: 2024-10-22T10:05:54.014Z
Learning: The `__eq__` method in `ParticleData` uses `self.dataset == that.dataset` for dataset comparison, which might not be sufficient for numpy arrays as it returns an array of booleans. The use of `np.array_equal` is suggested for a more robust comparison.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/patchdata.py
🧬 Code graph analysis (14)
src/core/utilities/types.hpp (1)
src/amr/data/field/field_data.hpp (2)
max(475-486)max(475-476)
src/amr/messengers/hybrid_messenger_strategy.hpp (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(412-421)ions(412-412)ions(425-450)ions(425-425)ions(452-487)ions(452-452)ions(490-502)ions(490-490)ions(513-552)ions(513-514)level(587-611)level(587-589)level(717-728)level(717-717)level(914-929)level(914-914)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (3)
pyphare/pyphare/pharesee/geometry.py (1)
hierarchy_overlaps(297-307)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
zeros_like(617-631)has_non_zero(633-646)pyphare/pyphare/core/box.py (4)
amr_to_local(182-183)shift(107-108)select(186-187)DataSelector(189-203)
src/amr/level_initializer/hybrid_level_initializer.hpp (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(412-421)ions(412-412)ions(425-450)ions(425-425)ions(452-487)ions(452-452)ions(490-502)ions(490-490)ions(513-552)ions(513-514)level(587-611)level(587-589)level(717-728)level(717-717)level(914-929)level(914-914)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (3)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
zeros_like(194-201)pyphare/pyphare/core/box.py (2)
copy(68-69)copy(138-142)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
copy(43-47)
src/amr/messengers/hybrid_messenger.hpp (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(412-421)ions(412-412)ions(425-450)ions(425-425)ions(452-487)ions(452-452)ions(490-502)ions(490-490)ions(513-552)ions(513-514)level(587-611)level(587-589)level(717-728)level(717-717)level(914-929)level(914-914)src/amr/messengers/hybrid_messenger_strategy.hpp (6)
ions(83-84)ions(88-89)ions(131-132)ions(134-135)ions(137-138)level(121-121)
src/core/data/field/field_box.hpp (2)
src/amr/data/field/field_data.hpp (4)
src(311-311)src(316-316)max(475-486)max(475-476)src/amr/data/tensorfield/tensor_field_data.hpp (4)
src(339-339)src(343-343)max(525-536)max(525-526)
src/amr/messengers/refiner.hpp (1)
src/amr/messengers/communicator.hpp (6)
algo(88-97)algo(88-88)algo(105-109)algo(105-106)algo(112-116)algo(112-113)
pyphare/pyphare/core/box.py (2)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (2)
select(94-116)select(233-234)src/core/utilities/box/box.hpp (1)
upper(96-96)
src/amr/data/tensorfield/tensor_field_data.hpp (2)
src/amr/data/field/field_data.hpp (22)
src(311-311)src(316-316)overlap(196-199)overlap(196-196)overlap(408-425)overlap(408-408)stream(207-238)stream(207-208)stream(245-249)stream(245-246)stream(312-313)stream(317-318)unpackStreamAndMax(466-472)unpackStreamAndMax(466-467)unpackStream(252-280)unpackStream(252-253)max(475-486)max(475-476)copy_(335-351)copy_(335-337)copy_(360-402)copy_(360-360)src/amr/messengers/field_max_transaction.hpp (5)
stream(63-63)stream(65-65)stream(69-69)unpackStream(149-159)unpackStream(149-149)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
zeros_like(617-631)
tests/functional/harris/harris_2d_diff.py (5)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/pharesee/run/run.py (9)
Run(35-293)GetRanks(179-188)filename(191-192)GetB(92-100)GetNi(116-118)GetMassDensity(112-114)GetE(102-110)times(292-293)all_times(275-290)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
diff_hierarchy(615-670)tests/simulator/__init__.py (1)
SimulatorTest(200-266)pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
hierarchy_from(13-46)
src/amr/data/field/field_data.hpp (2)
src/amr/messengers/field_max_transaction.hpp (2)
unpackStream(149-159)unpackStream(149-149)src/amr/messengers/field_sum_transaction.hpp (5)
unpackStream(162-172)unpackStream(162-162)stream(76-76)stream(78-78)stream(82-82)
src/amr/messengers/field_max_transaction.hpp (2)
src/amr/messengers/field_sum_transaction.hpp (11)
dst_level(206-228)dst_level(207-212)stream(76-76)stream(78-78)stream(82-82)packStream(153-159)packStream(153-153)unpackStream(162-172)unpackStream(162-162)level(230-237)level(231-232)src/amr/data/field/field_data.hpp (12)
overlap(196-199)overlap(196-196)overlap(408-425)overlap(408-408)stream(207-238)stream(207-208)stream(245-249)stream(245-246)stream(312-313)stream(317-318)unpackStream(252-280)unpackStream(252-253)
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
[notice] 617-617: Cyclic import
Import of module pyphare.pharesee.geometry begins an import cycle.
🪛 Ruff (0.14.7)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
620-620: Prefer next(iter(diff_hier.time_hier.keys())) over single element slice
Replace with next(iter(diff_hier.time_hier.keys()))
(RUF015)
626-626: Avoid specifying long messages outside the exception class
(TRY003)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
624-624: Prefer next(iter(copy.time_hier.values())) over single element slice
Replace with next(iter(copy.time_hier.values()))
(RUF015)
626-626: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
638-638: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
640-640: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
642-642: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
653-653: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
658-658: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
pyphare/pyphare/core/box.py
187-187: Ambiguous variable name: l
(E741)
187-187: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
tests/functional/harris/harris_2d_diff.py
1-1: Shebang is present but file is not executable
(EXE001)
60-60: Unused function argument: x
(ARG001)
68-68: Ambiguous variable name: l
(E741)
103-103: Unused function argument: x
(ARG001)
103-103: Unused function argument: y
(ARG001)
115-115: Unused function argument: x
(ARG001)
115-115: Unused function argument: y
(ARG001)
118-118: Unused function argument: x
(ARG001)
118-118: Unused function argument: y
(ARG001)
121-121: Unused function argument: x
(ARG001)
121-121: Unused function argument: y
(ARG001)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
215-215: f-string without any placeholders
Remove extraneous f prefix
(F541)
230-230: f-string without any placeholders
Remove extraneous f prefix
(F541)
278-278: Test for membership should be not in
Convert to not in
(E713)
⏰ 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). (5)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (36)
pyphare/pyphare/core/gridlayout.py (1)
12-121: Consistent addition of"value": "primal"entries inyee_centeringThe new
"value": "primal"entries for all directions are consistent and integrate cleanly withyee_centering_lowerandyee_element_is_primal, giving a straightforward way to treat a generic"value"quantity as primal without impacting existing logic in this file. Looks good as-is.pyphare/pyphare/simulator/simulator.py (1)
84-88: Defaultingprint_one_lineto False looks consistent; just confirm expected UX/logging impactDocstring and implementation now agree that single‑line progress printing is opt‑in, with
print_eolstaying"\n"by default andprint_eolexplicitly overridingprint_one_linewhen provided. Behavior is coherent; the only concern is that this is a silent default change that may alter CLI/log formatting for existing callers relying on the previous default. Worth double‑checking that this shift (more verbose, line‑by‑line output) is intended for all current usages.Also applies to: 100-103
src/amr/messengers/field_sum_transaction.hpp (1)
1-4: Header guard and include path refactor look good.The header guard follows the full directory path convention per project standards. The include path simplification from
core/utilities/logger/logger_defaults.hpptocore/logger.hppis cleaner and maintains consistency with the logging macros used throughout the file (PHARE_LOG_SCOPE at lines 100, 121, 131, 141, 148, 155, 164, 178, 185, 225, 234).Please verify that
core/logger.hppexports all the logging macros and utilities previously available fromcore/utilities/logger/logger_defaults.hppto ensure no compilation or runtime issues arise.pyphare/pyphare/pharesee/geometry.py (3)
211-219: LGTM!The addition of the
"name"field to intra-domain overlap dictionaries is consistent with the broader pattern in this function. Theref_pdnamevariable is correctly in scope from the enclosing loop.
221-230: LGTM!The
appendhelper function is correctly updated to accept and include thenameparameter, maintaining consistency with the intra-domain overlap dictionary structure.
275-292: LGTM!Both calls to
appendcorrectly passref_pdnameas the first argument, ensuring border-induced overlaps have the same"name"field as intra-domain overlaps.src/core/utilities/mpi_utils.hpp (1)
162-165: Excellent defensive programming for zero-size segments.Explicitly checking for zero-size segments before constructing the vector from a pointer range prevents potential undefined behavior when
offsetcould equalrcvBuff.size(). This is safer and clearer than relying on identical begin/end pointers.pyphare/pyphare/pharesee/run/run.py (1)
287-288: LGTM! Safe guard prevents KeyError.The conditional check ensures that only known diagnostics are mapped, preventing potential KeyError when processing diagnostic files not listed in
quantities_per_file.tests/functional/harris/harris_2d_diff.py (1)
294-309: LGTM!The test class follows the established
SimulatorTestpattern with proper tearDown cleanup and MPI initialization. Thetest_runmethod correctly runs the simulation and validates diagnostics.src/core/data/tensorfield/tensorfield.hpp (1)
208-211: LGTM!Formatting-only change adding visual separation before the private section.
src/core/utilities/types.hpp (1)
540-545: LGTM!The
SetMaxfunctor follows the established pattern ofEqualsandPlusEqualsabove, and is correctly used inFieldData::max()andTensorFieldData::max()methods.src/amr/level_initializer/hybrid_level_initializer.hpp (1)
113-116: LGTM!The
fillIonBorderscall is correctly placed after computing charge density and bulk velocity, synchronizing the ion borders via max-refiners before proceeding to J/E computations. This maintains physical consistency at patch boundaries.src/amr/physical_models/hybrid_model.hpp (1)
182-186: LGTM - Max border fields registration for ions.The addition of
massDensity(),chargeDensity(), andvelocity()to the max-border field collections aligns with the new max-border synchronization infrastructure. Note thatchargeDensity()appears in bothsumBorderFields(line 180, for populations) andmaxBorderFields(line 184, for aggregate ions) - please verify this dual registration is intentional for the different border operations.src/amr/messengers/hybrid_messenger_strategy.hpp (1)
137-139: LGTM - Consistent interface extension.The new pure virtual
fillIonBordersmethod follows the established pattern offillFluxBordersandfillDensityBorders, maintaining interface consistency.src/amr/solvers/solver_ppc.hpp (1)
583-584: LGTM - Ion border fill placement after ion update.The call to
fillIonBordersis correctly positioned afterionUpdater_.updateIons(), ensuring that the max-border synchronization operates on fully updated ion data.src/amr/messengers/hybrid_messenger.hpp (1)
359-363: LGTM - Consistent delegation pattern.The
fillIonBordersmethod correctly delegates to the strategy, following the established pattern offillFluxBordersandfillDensityBorders.src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (1)
126-131: LGTM - Appropriate stub implementation.The empty override for
fillIonBordersis consistent with other no-op fill methods inMHDHybridMessengerStrategy, which is expected since MHD-Hybrid transitions don't require ion-specific border handling.src/amr/messengers/hybrid_messenger_info.hpp (1)
69-72: LGTM - Clean data structure extension.The new
maxBorderFieldsandmaxBorderVecFieldsmembers are appropriately placed and follow the naming convention established bysumBorderFields. These will carry the field names for max-border operations.src/amr/messengers/refiner.hpp (2)
25-26: LGTM - Enum extension for max-border types.The new
PatchFieldBorderMaxandPatchVecFieldBorderMaxenum values follow the established naming convention, mirroring the existing*BorderSumvariants.
204-208: Good addition of fallback error handling.The
elsebranch withruntime_errorprovides a safety net for unhandledRefinerTypevalues, which improves maintainability as new types are added.src/amr/data/tensorfield/tensor_field_data.hpp (2)
343-345: LGTM!The new
maxandunpackStreamAndMaxdeclarations are consistent with the existingsumandunpackStreamAndSumpattern in this class.
514-536: LGTM!The
unpackStreamAndMaxandmaximplementations correctly mirror the existing sum-path patterns, usingSetMaxOpinstead ofPlusEqualOp. The implementation properly:
- Uses
core::SetMax<value_type>as the operator- Delegates to the templated
unpackStreamandcopy_methods- Includes dimension equality assertion via
TBOX_ASSERT_OBJDIM_EQUALITY2- Performs proper dynamic casts for overlap and source data
This aligns well with the analogous implementation in
src/amr/data/field/field_data.hpp.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (5)
7-7: LGTM!The include for
core/utilities/types.hppis necessary to access theSetMaxutility used by the new max-border operations.
250-253: LGTM!The registration of
ionFluxBorderMaxRefiners_andionDensityBorderMaxRefiners_follows the established pattern used for the sum refiners above.
456-456: LGTM!Good defensive assertion to ensure the refiner count is consistent with the ion population count.
879-894: LGTM!The registration of max-border refiners mirrors the existing sum-border refiner pattern. The assertions at lines 881 and 888 ensure configuration consistency with the hardcoded checks in
fillIonBorders.
1045-1046: LGTM!The new type aliases and member vectors for max-border refiners follow the established naming and declaration patterns used by the sum-border refiners.
Also applies to: 1055-1056
src/amr/data/field/field_data.hpp (3)
101-101: LGTM!The const-correctness improvements on
BoxOverlapandPatchDataparameters are appropriate and align these method signatures with immutability expectations.Also applies to: 157-158, 175-175, 196-196, 208-208, 246-246, 253-253
315-319: LGTM!The new
maxandunpackStreamAndMaxdeclarations are consistent with the existingsumandunpackStreamAndSummethod pair.
465-486: LGTM!The
unpackStreamAndMaxandmaximplementations correctly follow the established sum-path pattern:
- Uses
core::SetMax<value_type>as the operator- Delegates to the templated
unpackStreamandcopy_methods- Includes proper dimension equality assertion
- Performs correct dynamic casts for overlap and source data
This implementation will be called by
FieldBorderMaxTransaction::unpackStreamandcopyLocalDataas shown in the relevant code snippets.src/amr/messengers/field_max_transaction.hpp (6)
20-81: LGTM!The class declaration follows the established pattern from
FieldBorderSumTransaction. Constructor validation is thorough with appropriate assertions, and member variables are correctly initialized.
84-123: LGTM!The message size computation methods correctly delegate to the underlying patch data's
getDataStreamSizeandcanEstimateStreamSizeFromBoxmethods, consistent with the sum transaction pattern.
139-159: LGTM!The
packStreamdelegates to the source patch data correctly. TheunpackStreamproperly casts toFieldData_tand invokesunpackStreamAndMax, which applies the max operation during unpacking.
169-186: LGTM!The
copyLocalDataimplementation correctly retrieves both source and destination patch data, casts them toFieldData_t, and applies themaxoperation over the overlap region.
189-225: LGTM!The
FieldBorderMaxTransactionFactorycorrectly implements the SAMRAIRefineTransactionFactoryinterface. Theallocatemethod performs appropriate validation and constructsFieldBorderMaxTransactioninstances. ThepreprocessScratchSpaceno-op is consistent with the sum transaction factory.
162-186:printClassDatathrows unconditionally - verify if this is intentional.The method throws
std::runtime_errorrather than providing debug output. Confirm whether this matches the implementation pattern in similar transaction classes and whether providing actual debug output is needed for this method.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
194-201: Avoid full dataset read inzeros_like; use metadata shape/dtype insteadThe current
zeros_likeimplementation does a fullself.dataset[:]read just to obtain the shape:copy.dataset = np.zeros(deepcopy(self.dataset[:].shape))For large, HDF5‑backed fields this is a major and unnecessary I/O + memory hit. You only need shape (and ideally dtype), which are available from the dataset metadata.
A more efficient version that keeps the intended semantics would be:
def zeros_like(self): from copy import deepcopy copy = deepcopy(self) assert id(copy.dataset) == id(self.dataset) shape = getattr(self.dataset, "shape", None) dtype = getattr(self.dataset, "dtype", None) copy.dataset = np.zeros(shape, dtype=dtype) assert id(copy.dataset) != id(self.dataset) return copyThis preserves the geometry and creates a fresh zeroed array without ever reading the underlying data.
Based on learnings, this repeats a previously flagged performance issue around loading
self.dataset[:]just for its shape.pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
615-670: Cyclic import and minor inefficiencies already flagged.The issues in this function were previously identified:
- Line 617: Cyclic import with
pyphare.pharesee.geometry(CodeQL warning)- Line 620: Use
next(iter(diff_hier.time_hier.keys()))instead of list slice- Line 658:
if np.any(diff):is simpler thanif len(np.nonzero(diff)[0]):- Lines 667-668: Redundant
has_non_zero()check after per-patch assertionspyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
648-662: Bug:max()returns incorrect results for negative-only data.Initializing
valwith zeros means if all values in a level are negative, the reported max will incorrectly be0. This was previously flagged and the suggested fix initializes withNoneor-np.inf.
🧹 Nitpick comments (9)
pyphare/pyphare/core/box.py (1)
183-204: Box‑based selection and DataSelector wrapper look fine; minor lint/style nits
select(data, box)correctly turns an inclusive[lower, upper]Box into NumPy slices, andDataSelectorgives a convenientDataSelector(data)[box]/[...] = valAPI without extra copies. This is a clean, minimal addition.If you want to keep Ruff happy and slightly improve readability:
- Rename the loop variable in
selectto avoidl(e.g.loorlower_i) and,- Since
Box.__init__already enforceslower.shape == upper.shape, you can addstrict=Truetozipif your supported Python version allows it:def select(data, box): return data[tuple( slice(lo, hi + 1) for lo, hi in zip(box.lower, box.upper, strict=True) )]None of this changes behavior; it just quiets linters and clarifies intent.
tests/functional/harris/harris_2d_diff.py (2)
31-315: Consider turning printed diffs into quantitative assertionsThe
diff()/check_diags()flow currently just prints max B/ρ/E differences and always passes unless an exception is raised. That’s useful for exploratory runs, but as a regression/functional test it won’t catch degradations in the Harris equilibrium.If the goal is to enforce near‑equilibrium numerically, consider adding simple checks like:
assert differ.max() < tol_Bfor B,assert differ.max() < tol_rhofor ion charge/mass densities,assert differ.max() < tol_Efor E,with explicit tolerances. That would turn this into a genuine guardrail instead of a smoke test that only fails on I/O errors.
168-260: Minor test‑side cleanups (optional): f‑strings and membership test styleA few low‑impact polish points, mostly to satisfy Ruff and keep the test lean:
In
plot_file_for_qty, you can simplify the nested formatting:def plot_file_for_qty(plot_dir, qty, time, extra=""): return f"{plot_dir}/harris_t{time:.10f}_{qty}_{extra}.png"Remove redundant
fprefixes where there’s no interpolation, e.g.:filename=plot_file_for_qty(plot_dir, "ranks", new_time, f"L{ilvl}") # similarly for "ionCharge", "ionMass"Prefer
if "Unable to synchronously open object" not in err:overif not "..." in err:inpost_advance.These are cosmetic only; they won’t affect behavior but will quiet the listed lint warnings.
src/core/data/field/field_box.hpp (1)
61-83:max_of_fieldsis correct; consider reusingoperate_on_fieldsand adding<algorithm>The new
max_of_fieldshelper does the right thing—element‑wisedst = max(dst, src)over matchinglcl_boxranges with an assertion on equal sizes. That fits well with the rest of the field‑box utilities.Two small, optional tweaks:
To avoid duplicating the iteration logic, you could implement this in terms of
operate_on_fieldsand the existingcore::SetMaxfunctor:template<typename FieldBoxT> void max_of_fields(FieldBoxT& dst, FieldBoxT const& src) { operate_on_fields<core::SetMax<typename FieldBoxT::value_type>>(dst, src); }(or similar, depending on how you expose
value_type).Since you’re using
std::max, explicitly including<algorithm>here would make the dependency local instead of relying on transitive includes.Both are non‑functional cleanups; the current implementation is already correct.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
617-631: Minor: Use iterator instead of list slice.The
list(...)[0]pattern creates unnecessary temporary lists. Also,ilvlis unused in the loop.- hier = (list(copy.time_hier.values()))[0] + hier = next(iter(copy.time_hier.values())) - for ilvl, lvl in hier.items(): + for lvl in hier.values():
633-646: Minor: Simplify has_non_zero implementation.Similar inefficiencies: unused loop variables and
np.any()would be cleaner.- hier = (list(self.time_hier.values()))[0] + hier = next(iter(self.time_hier.values())) - for ilvl, lvl in hier.items(): + for lvl in hier.values(): for patch in lvl: - for key, pd in patch.patch_datas.items(): - check = np.nonzero(pd.dataset[:]) - if len(check[0]): + for pd in patch.patch_datas.values(): + if np.any(pd.dataset[:]): return Truesrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
250-253: Max-border refiner wiring looks consistent; consider tightening invariantsThe new max-border machinery is wired coherently end‑to‑end:
- Type aliases and member vectors (
FieldGhostMaxRefinerPool,VecFieldGhostMaxRefinerPool,ionDensityBorderMaxRefiners_,ionFluxBorderMaxRefiners_) mirror the existing sum-based pools.registerInitCommspopulates the pools frominfo->maxBorderFields(2 scalars) andinfo->maxBorderVecFields(1 vector), with appropriate ghost overlap fill patterns.registerLevelforwards level registration to all max refiners, in line with the sum refiners.fillIonBorderssimply invokesfillon all max refiners, which is exactly what you want for a max reduction (no scratch-copy round trip needed as withsumVec_/sumField_).The hard-coded expectations in
fillIonBorders:assert(ionFluxBorderMaxRefiners_.size() == 1); assert(ionDensityBorderMaxRefiners_.size() == 2);are correct for the current Harris use case but bake in magic constants. If you expect this to evolve (e.g., more max-tracked fields), consider:
- Asserting against
info-side sizes (or a namedconstexprcount) rather than raw literals, or- Storing the expected counts once when building the pools and asserting against those, so the invariants remain obvious at the single source of truth.
Functionally this is fine; the suggestion is purely to make future refactors safer and more self‑documenting.
Also applies to: 490-502, 881-894, 1045-1047, 1055-1057
452-459: New density-border sanity check is helpful; optional stronger assertionThe added assertion
assert(popDensityBorderSumRefiners_.size() % ions.size() == 0);usefully encodes the implicit assumption that the number of density-border refiners is an integer multiple of the number of ion populations.
Since the loop below always uses
i * fieldsPerPopandi * fieldsPerPop + 1, you still implicitly require at least two fields per population. If there is any chanceinfo->sumBorderFieldschanges shape later, you might optionally tighten this to:assert(popDensityBorderSumRefiners_.size() == 2 * ions.size());or at least
fieldsPerPop >= 2, to fail fast if the mapping between fields and populations ever drifts.src/amr/messengers/field_max_transaction.hpp (1)
1-52: Interface and guard are sound; consideroverrideand defaulted dtorThe transaction class and factory are structured consistently with the existing sum variant, and the header guard follows the PHARE path-based naming convention; no functional issues here.
For cleanliness and future-proofing, you might:
- Mark all overridden virtuals with
override(and drop redundantvirtual), including the destructor:~FieldBorderMaxTransaction() override = default; bool canEstimateIncomingMessageSize() override; size_t computeIncomingMessageSize() override; ... void printClassData(std::ostream& stream) const override;
- Optionally replace the
/** ... TODO */class doc with a brief sentence explaining that this is the max-border analogue ofFieldBorderSumTransactionto avoid “TODO” surviving long-term.These are minor, non-blocking polish items.
Also applies to: 71-81, 190-225
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
pyphare/pyphare/core/box.py(1 hunks)pyphare/pyphare/core/gridlayout.py(3 hunks)pyphare/pyphare/pharesee/geometry.py(3 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/patchdata.py(1 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)pyphare/pyphare/simulator/simulator.py(2 hunks)src/amr/data/field/field_data.hpp(8 hunks)src/amr/data/tensorfield/tensor_field_data.hpp(2 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(1 hunks)src/amr/messengers/field_max_transaction.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(7 hunks)src/amr/messengers/hybrid_messenger.hpp(1 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/refiner.hpp(6 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(1 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/mpi_utils.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)tests/functional/harris/harris_2d_diff.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/amr/physical_models/hybrid_model.hpp
- src/core/data/tensorfield/tensorfield.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/core/utilities/types.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/messengers/hybrid_messenger.hpp
- pyphare/pyphare/simulator/simulator.py
- pyphare/pyphare/core/gridlayout.py
- src/amr/data/field/field_data.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/data/tensorfield/tensor_field_data.hppsrc/core/utilities/mpi_utils.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/messengers/refiner.hppsrc/amr/messengers/field_sum_transaction.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/field_max_transaction.hppsrc/core/data/field/field_box.hpp
🧠 Learnings (4)
📓 Common learnings
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: 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: 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/messengers/refiner.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/field_max_transaction.hpp
📚 Learning: 2024-10-18T13:23:32.074Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
📚 Learning: 2024-10-22T10:05:54.014Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 0
File: :0-0
Timestamp: 2024-10-22T10:05:54.014Z
Learning: The `__eq__` method in `ParticleData` uses `self.dataset == that.dataset` for dataset comparison, which might not be sufficient for numpy arrays as it returns an array of booleans. The use of `np.array_equal` is suggested for a more robust comparison.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/patchdata.py
🧬 Code graph analysis (7)
src/amr/data/tensorfield/tensor_field_data.hpp (2)
src/amr/data/field/field_data.hpp (22)
src(311-311)src(316-316)overlap(196-199)overlap(196-196)overlap(408-425)overlap(408-408)stream(207-238)stream(207-208)stream(245-249)stream(245-246)stream(312-313)stream(317-318)unpackStreamAndMax(466-472)unpackStreamAndMax(466-467)unpackStream(252-280)unpackStream(252-253)max(475-486)max(475-476)copy_(335-351)copy_(335-337)copy_(360-402)copy_(360-360)src/amr/messengers/field_max_transaction.hpp (5)
stream(63-63)stream(65-65)stream(69-69)unpackStream(149-159)unpackStream(149-149)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
zeros_like(194-201)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
copy(43-47)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
zeros_like(617-631)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
copy(43-47)
tests/functional/harris/harris_2d_diff.py (4)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
diff_hierarchy(615-670)tests/simulator/__init__.py (1)
SimulatorTest(200-266)tests/simulator/test_advance.py (1)
AdvanceTestBase(33-869)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
src/amr/messengers/field_max_transaction.hpp (1)
src/amr/messengers/field_sum_transaction.hpp (25)
dst_level(206-228)dst_level(207-212)stream(76-76)stream(78-78)stream(82-82)canEstimateIncomingMessageSize(98-115)canEstimateIncomingMessageSize(98-98)getSourceProcessor(139-143)getSourceProcessor(139-139)computeIncomingMessageSize(119-126)computeIncomingMessageSize(119-119)computeOutgoingMessageSize(129-136)computeOutgoingMessageSize(129-129)getDestinationProcessor(146-150)getDestinationProcessor(146-146)packStream(153-159)packStream(153-153)unpackStream(162-172)unpackStream(162-162)printClassData(176-180)printClassData(176-176)copyLocalData(183-199)copyLocalData(183-183)level(230-237)level(231-232)
src/core/data/field/field_box.hpp (2)
src/amr/data/field/field_data.hpp (4)
src(311-311)src(316-316)max(475-486)max(475-476)src/amr/data/tensorfield/tensor_field_data.hpp (4)
src(339-339)src(343-343)max(525-536)max(525-526)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (3)
pyphare/pyphare/pharesee/geometry.py (1)
hierarchy_overlaps(297-307)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
zeros_like(617-631)has_non_zero(633-646)pyphare/pyphare/core/box.py (2)
amr_to_local(182-183)shift(107-108)
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
[notice] 617-617: Cyclic import
Import of module pyphare.pharesee.geometry begins an import cycle.
🪛 Ruff (0.14.7)
pyphare/pyphare/core/box.py
187-187: Ambiguous variable name: l
(E741)
187-187: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
624-624: Prefer next(iter(copy.time_hier.values())) over single element slice
Replace with next(iter(copy.time_hier.values()))
(RUF015)
626-626: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
638-638: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
640-640: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
642-642: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
653-653: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
tests/functional/harris/harris_2d_diff.py
1-1: Shebang is present but file is not executable
(EXE001)
60-60: Unused function argument: x
(ARG001)
68-68: Ambiguous variable name: l
(E741)
103-103: Unused function argument: x
(ARG001)
103-103: Unused function argument: y
(ARG001)
115-115: Unused function argument: x
(ARG001)
115-115: Unused function argument: y
(ARG001)
118-118: Unused function argument: x
(ARG001)
118-118: Unused function argument: y
(ARG001)
121-121: Unused function argument: x
(ARG001)
121-121: Unused function argument: y
(ARG001)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
218-218: f-string without any placeholders
Remove extraneous f prefix
(F541)
233-233: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: Test for membership should be not in
Convert to not in
(E713)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
620-620: Prefer next(iter(diff_hier.time_hier.keys())) over single element slice
Replace with next(iter(diff_hier.time_hier.keys()))
(RUF015)
626-626: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (16)
pyphare/pyphare/pharesee/run/run.py (1)
275-289: Guarding against unknown diagnostic files looks correctWrapping the assignment in
if basename in quantities_per_filepreventsKeyErrorwhen extra.h5files are present while preserving behavior for known diagnostics. This is a sensible robustness improvement; just ensurequantities_per_filestays in sync with any new “core” diags you care about exposing viaall_times().If you want to verify coverage, you can quickly inspect which basenames appear in the current output directory and compare them to
quantities_per_file.src/amr/messengers/field_sum_transaction.hpp (1)
4-4: Logger include switch is consistent and non‑disruptiveUsing
"core/logger.hpp"here aligns this transaction with the central logging header used elsewhere. Given the existingPHARE_LOG_SCOPEcalls, this is the right dependency and should not affect behavior.src/amr/solvers/solver_ppc.hpp (1)
547-589: Ion border fill ordering looks reasonable; confirm intended semanticsCalling
fillIonBordersafterionUpdater_.updateIons(state.ions)fits the comment that Ni/Vi must be computed before filling pure ghost nodes and mirrors the pattern used for other ghost/border fills. Given the existing sequence:
- updatePopulations
- setTime(ions)
- fill flux/density/moment/ghost-particle borders
updateIons- now
fillIonBordersthis should give border code the fully-updated ion state at
newTime. Please just confirm thatfillIonBordersis indeed defined in terms of the post‑updateIonsquantities and doesn’t need to run earlier in the sequence.src/core/utilities/mpi_utils.hpp (1)
145-169: Zero‑length segment handling incollectVectorcorrectly avoids UBBranching on
perMPISize[i] == 0and emplacing an emptyVectoravoids constructing from&rcvBuff[offset]when the global receive buffer is empty (e.g., all ranks have size 0). For non‑zero entries the behavior is unchanged. This is a solid fix for the all‑zero case without impacting existing usage.src/amr/messengers/refiner.hpp (3)
25-26: LGTM: New RefinerType enum values for max-based border synchronization.The new
PatchFieldBorderMaxandPatchVecFieldBorderMaxenum values follow the established pattern for sum-based refiners.
204-208: Good defensive programming: fallback error for unhandled RefinerType.The final else branch ensures that if a new
RefinerTypeis added but not handled inregisterLevel, a clear runtime error is thrown rather than silently producing undefined behavior.
115-137: Unable to verify review comment due to repository access constraints.The review comment suggests adding
PatchTensorFieldBorderMaxfor consistency with existing tensor field border operations. However, verification cannot be completed because the repository cannot be cloned in the sandbox environment. The claim thatPatchTensorFieldBorderSumexists at lines 95-103 and thatPatchTensorFieldBorderMaxis missing requires direct inspection of the source file to confirm. Additionally, determining whether this represents a genuine design gap or an incorrect assumption requires understanding the actual usage patterns of tensor fields in the codebase and the RefinerType enum definition.pyphare/pyphare/pharesee/geometry.py (3)
211-219: LGTM: Adding "name" field to overlap dictionaries.The addition of
"name": ref_pdnameto overlap entries enables consumers (likediff_hierarchy) to identify which patch data quantity the overlap corresponds to without needing to inspect the pdatas directly.
221-230: LGTM: Updated append helper signature.The
nameparameter addition to the localappendhelper is consistent with the intra-domain overlap changes and maintains the same dictionary structure for all overlap entries.
275-292: LGTM: Consistent usage of the updated append function.Both calls to
appendnow correctly passref_pdnameas the first argument, ensuring all border-induced overlaps include the "name" field.src/amr/data/tensorfield/tensor_field_data.hpp (2)
343-345: LGTM: Method declarations follow established patterns.The new
maxandunpackStreamAndMaxdeclarations mirror the existingsumandunpackStreamAndSumdeclarations, maintaining API consistency.
514-536: SetMax implementation and consistency with sum operations requires codebase verification.The code snippet shows clean, correct syntax with proper type aliasing and method structure. However, without access to the PHARE repository, I cannot verify:
- The
SetMaxdefinition in core utilities- Consistency with
SetSumand sum operation counterparts- Pattern alignment with
field_data.hppThe original approval is reasonable based on the snippet's quality, but the specific verification claims remain unconfirmed.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
4-9: Include ofcore/utilities/types.hppis appropriateBringing in
core/utilities/types.hpphere is reasonable given the use of PHARE type utilities (e.g., tensor helpers) in this strategy; no issues from a cleanliness or dependency perspective.src/amr/messengers/field_max_transaction.hpp (3)
85-123: Message-size estimation and streaming mirror the sum transaction correctly
canEstimateIncomingMessageSize,computeIncomingMessageSize,computeOutgoingMessageSize,getSourceProcessor,getDestinationProcessor, andpackStreamall follow the same pattern as the sum-border transaction:
- They use the appropriate patch level (
d_src_levelvsd_dst_level) andd_refine_data[d_item_id]->d_src/d_scratchindices.- They operate over
*d_overlapas expected for border exchanges.getSourceProcessor/getDestinationProcessordelegate to the owning ranks of the source/destination boxes, which is the right thing for SAMRAI’s transaction routing.Given that the implementation intentionally clones the existing sum-transaction logic, there are no obvious correctness or performance issues here.
Also applies to: 126-147
149-186: Max reduction semantics depend onFieldData_tAPI but look coherentThe core max logic:
onode_dst_data->unpackStreamAndMax(stream, *d_overlap); ... onode_dst_data->max(*onode_src_data, *d_overlap);is the natural analogue of the sum version and will do the right thing as long as
FieldData_timplements:
unpackStreamAndMax(MessageStream&, BoxOverlap const&), andmax(FieldData_t const&, BoxOverlap const&).This keeps the transaction layer thin and lets the field type own the element-wise max semantics, which matches the existing design for sums.
printClassDatathrowing is also consistent with the sum transaction’s “should not be called” stance.Overall the max reduction path is conceptually correct; just ensure the
FieldData_tspecializations used with this header provide those two methods.
217-224: Factory nooppreprocessScratchSpaceis fine and consistentThe factory’s
preprocessScratchSpaceimplementation is intentionally a no-op with logging, mirroring the sum-border factory. That keeps behavior predictable and avoids unexpected scratch mutations before the max transactions run; no changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pyphare/pyphare/pharein/simulation.py (1)
408-409: Clarify ghost-overlap comment and reuse existing array for default smallest_patch_sizeThe new default
smallest_patch_sizebased on2 * max_ghosts + 1makes sense for avoiding ghost overlap, but the comment is a bit misleading (“smallest_patch_size * 2 + 1”), and you recompute the same array you already stored insmall_invalid_patch_size.You can clarify intent and avoid the duplicate
np_array_ifycall with:- # to prevent primal ghost overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 - smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 + # default smallest_patch_size to 2 * max_ghosts + 1 to prevent primal ghost overlaps of non adjacent patches + smallest_patch_size = small_invalid_patch_size * 2 + 1Note that the validation below still only enforces
smallest_patch_size > small_invalid_patch_size(i.e. > max_ghosts), so user-specified values can be smaller than2 * max_ghosts + 1. If the “no overlap” condition is meant to hold even for customsmallest_patch_size, you may want to tighten that check accordingly; otherwise the current behavior is fine as an expert override.tools/diff_diags.py (4)
1-1: Align shebang with intended usage (EXE001)Ruff flags the shebang because the file isn’t executable. If this is meant to be run as
python tools/diff_diags.py, consider dropping the shebang; if it’s intended as a CLI script, make the file executable in the repo.
25-26: Simplify time formatting inplot_file_for_qtyYou can avoid the nested
.formatcall and make the f-string clearer:-def plot_file_for_qty(plot_dir, qty, time, extra=""): - return f"{plot_dir}/harris_t{"{:.10f}".format(time)}_{qty}_{extra}.png" +def plot_file_for_qty(plot_dir, qty, time, extra=""): + return f"{plot_dir}/harris_t{time:.10f}_{qty}_{extra}.png"This is a bit more direct while keeping the same behavior. Based on learnings, this keeps the helper minimal and efficient.
42-42: Remove unnecessaryfprefixes on constant strings (F541)Ruff’s F541 warnings come from f-strings without placeholders. These can be plain strings:
- filename=plot_file_for_qty(plot_dir, f"ranks", new_time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", new_time, f"L{ilvl}"), ... - filename=plot_file_for_qty( - plot_dir, f"ionCharge", new_time, f"L{ilvl}" - ), + filename=plot_file_for_qty( + plot_dir, "ionCharge", new_time, f"L{ilvl}" + ), ... - filename=plot_file_for_qty(plot_dir, f"ionMass", new_time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionMass", new_time, f"L{ilvl}"),This should clear the F541 diagnostics without changing behavior.
Also applies to: 96-96, 111-111
141-145: Usenot infor membership test (E713)Ruff suggests the canonical
not inform here:- if not "Unable to synchronously open object" in err: # no diag for time + if "Unable to synchronously open object" not in err: # no diag for timeSame logic, slightly clearer and matches the linter expectation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyphare/pyphare/pharein/simulation.py(1 hunks)tools/diff_diags.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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: 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: 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.
🧬 Code graph analysis (2)
pyphare/pyphare/pharein/simulation.py (2)
pyphare/pyphare/core/phare_utilities.py (1)
np_array_ify(62-67)pyphare/pyphare/core/gridlayout.py (1)
ndim(224-225)
tools/diff_diags.py (7)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/pharesee/run/run.py (7)
Run(35-293)GetRanks(179-188)GetE(102-110)GetB(92-100)GetNi(116-118)GetMassDensity(112-114)all_times(275-290)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
diff_hierarchy(615-670)tests/simulator/test_advance.py (2)
AdvanceTestBase(33-869)base_test_overlaped_fields_are_equal(217-387)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
levelNbr(222-225)plot_patches(342-355)levels(214-217)has_non_zero(633-646)pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
hierarchy_from(13-46)
🪛 Ruff (0.14.7)
tools/diff_diags.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: f-string without any placeholders
Remove extraneous f prefix
(F541)
96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
111-111: f-string without any placeholders
Remove extraneous f prefix
(F541)
144-144: Test for membership should be not in
Convert to not in
(E713)
⏰ 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). (5)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (1)
tools/diff_diags.py (1)
153-162: Diagnostic loop and MPI startup look goodThe
check_diags()+__main__wiring (usingRun(diag_dir).all_times()["B"]andstartMPI()once at entry) is straightforward and matches the surrounding test/Run abstractions; no functional issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
648-662: CRITICAL: Fixmax()to handle negative data.Line 654 initializes
valwith zeros, causingmax()to incorrectly report0when all values on a level are negative. Given that this PR involves signed fields (magnetic field B, electric field E per the PR description), this is a correctness bug that will produce invalid results for negative-only data.Example: If all data is
[-5, -3, -1], the current code reports0instead of the correct maximum-1.Apply this diff to fix the initialization and tighten the implementation:
def max(self, qty=None): """only works for fields and vecfields with 1 time""" assert len(self.time_hier) == 1 - hier = (list(self.time_hier.values()))[0] - val = [0 for ilvl, lvl in hier.items()] + hier = next(iter(self.time_hier.values())) + max_level = max(hier.keys()) + val = [None] * (max_level + 1) for ilvl, lvl in hier.items(): for patch in lvl: - for key, pd in patch.patch_datas.items(): - if qty is None or key == qty: - val[ilvl] = max(np.max(pd.dataset), val[ilvl]) + for key, pd in patch.patch_datas.items(): + if qty is None or key == qty: + level_max = np.max(pd.dataset[:]) + if val[ilvl] is None or level_max > val[ilvl]: + val[ilvl] = level_max return valThis preserves the public API (a list indexed by level number) while correctly handling negative-only data.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (4)
617-617: Cyclic import risk persists.This import has been flagged by CodeQL as creating a module cycle. The previous review already identified this issue.
620-620: Use iterator instead of list conversion.Replace
list(diff_hier.time_hier.keys())[0]withnext(iter(diff_hier.time_hier))to avoid creating an intermediate list.
658-658: Simplify non-zero check.
if np.any(diff):is cleaner and dimension-agnostic compared toif len(np.nonzero(diff)[0]):.
667-668: Expensive re-scan for final assertion.The
diff_hier.has_non_zero()call re-scans all datasets even though per-patch assertions already verified non-zeros. Consider removing or guarding behind a debug flag.pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
194-201: Inefficient full dataset read for shape metadata.This issue was already flagged in a previous review. Using
self.dataset[:].shapeforces a full HDF5 read just to obtain the shape. Useself.dataset.shapeandself.dataset.dtypedirectly from metadata instead:def zeros_like(self): from copy import deepcopy copy = deepcopy(self) assert id(copy.dataset) == id(self.dataset) - copy.dataset = np.zeros(deepcopy(self.dataset[:].shape)) + copy.dataset = np.zeros(self.dataset.shape, dtype=getattr(self.dataset, "dtype", None)) assert id(copy.dataset) != id(self.dataset) return copy
🧹 Nitpick comments (17)
pyphare/pyphare/pharesee/geometry.py (1)
211-230: Overlap dict structure updated; docstring now out of dateThe addition of
"name": ref_pdnameand theappend(name, ...)helper keeps the runtime behavior coherent and makes overlaps easier to label, but thecompute_overlapsdocstring above still documents only three keys (pdatas,box,offset). The actual structure now includes at least"name"and"patches"as well.Consider updating the docstring to list all keys so downstream users don’t rely on stale docs.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
617-631: Consider minor optimizations forzeros_like().The logic is correct, but a few optional refinements can improve readability and efficiency:
- Line 624: Use
next(iter(copy.time_hier.values()))instead oflist(...)[0]for better performance.- Line 626: The loop variable
ilvlis unused; iterate overhier.values()directly.Apply this diff:
def zeros_like(self): """only works for fields and vecfields with 1 time""" from copy import deepcopy assert len(self.time_hier) == 1 copy = deepcopy(self) - hier = (list(copy.time_hier.values()))[0] + hier = next(iter(copy.time_hier.values())) - for ilvl, lvl in hier.items(): + for lvl in hier.values(): for patch in lvl: for key, pd in patch.patch_datas.items(): patch.patch_datas[key] = pd.zeros_like() assert not copy.has_non_zero() return copy
633-646: Simplifyhas_non_zero()withnp.any()for efficiency.The current implementation using
np.nonzero()and length checks can be streamlined withnp.any(), which short-circuits on the first non-zero value and is more direct.Apply this diff:
def has_non_zero(self): """only works for fields and vecfields with 1 time""" assert len(self.time_hier) == 1 - hier = (list(self.time_hier.values()))[0] + hier = next(iter(self.time_hier.values())) - for ilvl, lvl in hier.items(): + for lvl in hier.values(): for patch in lvl: - for key, pd in patch.patch_datas.items(): - check = np.nonzero(pd.dataset[:]) - if len(check[0]): + for pd in patch.patch_datas.values(): + if np.any(pd.dataset[:]): return True return Falsepyphare/pyphare/core/box.py (1)
186-187: Rename ambiguous variableltoloorlower.The variable name
lcan be confused with1orI. A clearer name improves readability.def select(data, box): - return data[tuple([slice(l, u + 1) for l, u in zip(box.lower, box.upper)])] + return data[tuple([slice(lo, hi + 1) for lo, hi in zip(box.lower, box.upper)])]pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (2)
655-661: Consider consistent variable naming for clarity.
dif0/dif1(current values) vsdiff(computed difference) is slightly confusing. Consider renaming tocurr_diff0/curr_diff1or similar to clarify these are the existing values being compared against the newly computeddiff.- dif0 = boxm.select(diff_data0, box_pd1) - dif1 = boxm.select(diff_data1, box_pd2) + existing0 = boxm.select(diff_data0, box_pd1) + existing1 = boxm.select(diff_data1, box_pd2) if len(np.nonzero(diff)[0]): - boxm.DataSelector(diff_data0)[box_pd1] = np.maximum(dif0, diff) - boxm.DataSelector(diff_data1)[box_pd2] = np.maximum(dif1, diff) + boxm.DataSelector(diff_data0)[box_pd1] = np.maximum(existing0, diff) + boxm.DataSelector(diff_data1)[box_pd2] = np.maximum(existing1, diff)
628-628: Use boolean instead of int for flag.
found = Falseandfound = Truewould be more idiomatic for a boolean flag.- found = 0 + found = False for ilvl, overlaps in hierarchy_overlaps(hier, time).items(): ... - found = 1 + found = Truesrc/core/data/field/field_box.hpp (1)
71-82: Ensure<algorithm>is included forstd::max; optional alignment with generic opsThe elementwise max logic and iterator pattern look good and are consistent with
operate_on_fields. Two points:
- To keep this header self‑contained, it should not rely on transitive includes for
std::max. Ifcore/def.hppor other headers stop including<algorithm>, this will break. I’d recommend adding the include here:#include "core/utilities/box/box.hpp" #include <vector> #include <cstddef> #include <type_traits> + #include <algorithm> // for std::max
- Given the broader PR adds generic operator machinery (e.g.,
operate_on_fields<Operator>and aSetMax-like operation), you might consider expressing this helper via that path to avoid a special‑case implementation, if it doesn’t complicate call sites. If you feel that extra indirection hurts readability/minimalism, the current direct implementation is fine.src/amr/messengers/refiner.hpp (1)
122-132: Minor comment clarity suggestion.The comment "schedule used to == max" reads a bit awkwardly. Consider rephrasing to "schedule used to compute max" or "schedule used to set max values" for clarity.
- // schedule used to == max of density and flux for populations + // schedule used to set max of density and flux for populations // on complete overlaped ghost box nodessrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
490-502: Hard-coded size assertions may be fragile.The assertions
ionFluxBorderMaxRefiners_.size() == 1andionDensityBorderMaxRefiners_.size() == 2couple this implementation to a specific configuration. If the number of max-border fields changes, these assertions will fail without clear indication of why.Consider either:
- Removing the assertions if they're purely for debugging, or
- Adding a clarifying comment explaining why these specific sizes are expected, or
- Making the logic work with any valid size
void fillIonBorders(IonsT& ions, level_t& level, double const fillTime) override { - // - assert(ionFluxBorderMaxRefiners_.size() == 1); - assert(ionDensityBorderMaxRefiners_.size() == 2); + // Expects 1 flux refiner (ions.velocity()) and 2 density refiners (charge/mass density) + assert(ionFluxBorderMaxRefiners_.size() == 1 && "Expected 1 ion flux max refiner"); + assert(ionDensityBorderMaxRefiners_.size() == 2 && "Expected 2 ion density max refiners"); for (auto& refiner : ionFluxBorderMaxRefiners_) refiner.fill(level.getLevelNumber(), fillTime); for (auto& refiner : ionDensityBorderMaxRefiners_) refiner.fill(level.getLevelNumber(), fillTime); - - // }tools/diff_diags.py (7)
42-42: Remove extraneousfprefix from string literal.The string
f"ranks"has no placeholders, so thefprefix is unnecessary.- filename=plot_file_for_qty(plot_dir, f"ranks", new_time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", new_time, f"L{ilvl}"),
96-96: Remove extraneousfprefix from string literal.Same issue as above -
f"ionCharge"has no placeholders.- plot_dir, f"ionCharge", new_time, f"L{ilvl}" + plot_dir, "ionCharge", new_time, f"L{ilvl}"
111-111: Remove extraneousfprefix from string literal.Same issue -
f"ionMass"has no placeholders.- filename=plot_file_for_qty(plot_dir, f"ionMass", new_time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionMass", new_time, f"L{ilvl}"),
144-144: Usenot infor membership test.The idiomatic Python pattern is
not inrather thannot ... in.- if not "Unable to synchronously open object" in err: # no diag for time + if "Unable to synchronously open object" not in err: # no diag for time
129-129: Module-level test instantiation may cause import-time side effects.Creating
AdvanceTestBase(rethrow=True)at module import time could trigger unintended side effects. Consider moving this insidecheck_timeorcheck_diagsif the test object doesn't need to persist across calls.-test = AdvanceTestBase(rethrow=True) # change to False for debugging images - def check_time(new_time): + test = AdvanceTestBase(rethrow=True) # change to False for debugging images if cpp.mpi_rank() == 0:
139-148: Error handling silently swallows some exceptions.The
KeyErrorhandler only logs when the error message doesn't contain a specific string, meaning some legitimate errors might be silently ignored. Consider being more explicit about which errors are expected.
1-1: Shebang present but file is not executable.If this script is intended to be run directly (as indicated by the
if __name__ == "__main__":block), consider making it executable withchmod +x.src/amr/messengers/field_operate_transaction.hpp (1)
164-170:printClassDataalways throws - consider implementing or documenting.This method unconditionally throws a runtime error. If it's intentionally not implemented, consider either:
- Adding a
// TODOcomment explaining the intent- Providing a minimal implementation that prints class info
The documentation block at line 17 already says "TODO" which is helpful.
template<typename FieldData_t, typename Operation> void FieldBorderOpTransaction<FieldData_t, Operation>::printClassData(std::ostream& stream) const { PHARE_LOG_SCOPE(2, "FieldBorderOpTransaction::printClassData"); - - throw std::runtime_error("FieldBorderOpTransaction::printClassData!"); + // TODO: Implement proper class data printing + stream << "FieldBorderOpTransaction: not fully implemented\n"; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
pyphare/pyphare/core/box.py(1 hunks)pyphare/pyphare/core/gridlayout.py(3 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)pyphare/pyphare/pharesee/geometry.py(3 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/patchdata.py(1 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)pyphare/pyphare/simulator/simulator.py(2 hunks)src/amr/data/field/field_data.hpp(9 hunks)src/amr/data/tensorfield/tensor_field_data.hpp(3 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(1 hunks)src/amr/messengers/field_operate_transaction.hpp(7 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(7 hunks)src/amr/messengers/hybrid_messenger.hpp(1 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/refiner.hpp(7 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(1 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/mpi_utils.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(2 hunks)tests/simulator/__init__.py(1 hunks)tools/diff_diags.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/data/tensorfield/tensorfield.hpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/core/utilities/types.hpp
- src/core/utilities/mpi_utils.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- tests/simulator/init.py
- src/amr/physical_models/hybrid_model.hpp
- pyphare/pyphare/pharein/simulation.py
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- pyphare/pyphare/pharesee/run/run.py
🧰 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/messengers/hybrid_messenger.hppsrc/core/data/field/field_box.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/refiner.hppsrc/diagnostic/diagnostic_model_view.hppsrc/amr/messengers/field_operate_transaction.hppsrc/amr/data/field/field_data.hppsrc/amr/data/tensorfield/tensor_field_data.hpp
🧠 Learnings (4)
📓 Common learnings
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: 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: 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: 2024-10-18T13:23:32.074Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.pypyphare/pyphare/pharesee/hierarchy/hierarchy.py
📚 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/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/refiner.hppsrc/diagnostic/diagnostic_model_view.hppsrc/amr/messengers/field_operate_transaction.hpp
📚 Learning: 2024-10-22T10:05:54.014Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 0
File: :0-0
Timestamp: 2024-10-22T10:05:54.014Z
Learning: The `__eq__` method in `ParticleData` uses `self.dataset == that.dataset` for dataset comparison, which might not be sufficient for numpy arrays as it returns an array of booleans. The use of `np.array_equal` is suggested for a more robust comparison.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/patchdata.py
🧬 Code graph analysis (8)
src/amr/level_initializer/hybrid_level_initializer.hpp (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(412-421)ions(412-412)ions(425-450)ions(425-425)ions(452-487)ions(452-452)ions(490-502)ions(490-490)ions(513-552)ions(513-514)level(587-611)level(587-589)level(717-728)level(717-717)level(914-929)level(914-914)
src/amr/messengers/hybrid_messenger.hpp (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(412-421)ions(412-412)ions(425-450)ions(425-425)ions(452-487)ions(452-452)ions(490-502)ions(490-490)ions(513-552)ions(513-514)level(587-611)level(587-589)level(717-728)level(717-717)level(914-929)level(914-914)src/amr/messengers/hybrid_messenger_strategy.hpp (6)
ions(83-84)ions(88-89)ions(131-132)ions(134-135)ions(137-138)level(121-121)
pyphare/pyphare/core/box.py (3)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (2)
select(94-116)select(233-234)pyphare/pyphare/pharesee/particles.py (1)
select(102-135)src/core/utilities/box/box.hpp (1)
upper(96-96)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
zeros_like(617-631)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
copy(43-47)
src/amr/data/field/field_data.hpp (1)
src/amr/data/tensorfield/tensor_field_data.hpp (26)
source(114-157)source(114-114)source(180-189)source(180-180)source(382-385)source(382-382)overlap(218-221)overlap(218-218)overlap(454-482)overlap(454-454)unpackStream(284-318)unpackStream(284-285)stream(229-269)stream(229-230)stream(277-281)stream(277-278)unpackStreamAnd(346-347)unpackStreamAnd(491-495)unpackStreamAnd(491-492)operate(344-344)operate(500-509)operate(500-501)copy_(363-379)copy_(363-365)copy_(388-450)copy_(388-389)
src/amr/data/tensorfield/tensor_field_data.hpp (1)
src/amr/data/field/field_data.hpp (11)
overlap(200-203)overlap(200-200)overlap(411-428)overlap(411-411)operate(316-316)operate(452-461)operate(452-453)copy_(338-354)copy_(338-340)copy_(363-405)copy_(363-363)
tools/diff_diags.py (6)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/pharesee/run/run.py (8)
Run(35-293)GetRanks(179-188)filename(191-192)GetE(102-110)GetB(92-100)GetNi(116-118)GetMassDensity(112-114)times(292-293)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
diff_hierarchy(615-670)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
hierarchy_from(13-46)pyphare/pyphare/pharein/simulation.py (1)
check_time(142-187)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
zeros_like(194-201)
🪛 Ruff (0.14.8)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
620-620: Prefer next(iter(diff_hier.time_hier.keys())) over single element slice
Replace with next(iter(diff_hier.time_hier.keys()))
(RUF015)
626-626: Avoid specifying long messages outside the exception class
(TRY003)
pyphare/pyphare/core/box.py
187-187: Ambiguous variable name: l
(E741)
187-187: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
tools/diff_diags.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: f-string without any placeholders
Remove extraneous f prefix
(F541)
96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
111-111: f-string without any placeholders
Remove extraneous f prefix
(F541)
144-144: Test for membership should be not in
Convert to not in
(E713)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
624-624: Prefer next(iter(copy.time_hier.values())) over single element slice
Replace with next(iter(copy.time_hier.values()))
(RUF015)
626-626: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
638-638: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
640-640: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
642-642: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
653-653: Prefer next(iter(self.time_hier.values())) over single element slice
Replace with next(iter(self.time_hier.values()))
(RUF015)
⏰ 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: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (27)
pyphare/pyphare/simulator/simulator.py (2)
80-88: Docstring now matches runtime default forprint_one_lineThe updated description (“default False”) correctly reflects the new behavior and clarifies that single-line, in-place progress now has to be explicitly enabled. Looks consistent with the implementation below.
99-104: Behavior change: default progress output now line-based instead of in-placeBy switching
kwargs.get("print_one_line", False)to default False,run()will emit one line per advance unless callers explicitly setprint_one_line=True(or provide a customprint_eol). This looks intentional and the override order (print_eolkwarg winning overprint_one_line) is sensible; just be aware this may increase log verbosity compared to previous defaults.pyphare/pyphare/pharesee/geometry.py (1)
275-292: Consistent naming for border overlaps looks goodUsing
append(ref_pdname, ...)for bothoverlapandother_ovrlpensures border-derived overlaps carry the same"name"labeling as intra-domain entries, without changing existing offset semantics. This is a clean, minimal extension.pyphare/pyphare/core/box.py (1)
189-203: LGTM - clever pattern for enabling Box-based slice assignment.The
DataSelectorclass provides a clean API for assigning values to numpy array slices via Box indexing. The[:]assignment in__setitem__correctly modifies the underlying array view.pyphare/pyphare/core/gridlayout.py (1)
46-48:"value": "primal"entries look consistent; verify no unintended iteration effectsAdding
"value": "primal"for x/y/z keepsyee_centeringstructurally consistent with existing metadata patterns like"tags". However, confirm that any code iterating overyee_centering[direction]keys does not inadvertently treat"value"as a physical field rather than metadata.Also applies to: 82-84, 118-120
src/amr/level_initializer/hybrid_level_initializer.hpp (1)
116-116: LGTM!The new
fillIonBorderscall is correctly placed after ion moments are computed and at level scope (outside the patch loop), consistent with the existingfillFluxBordersandfillDensityBorderspattern.src/diagnostic/diagnostic_model_view.hpp (2)
9-9: LGTM!Header include updated to match the renamed transaction infrastructure.
173-180: LGTM!The refactoring from
FieldBorderSumTransactionFactoryto the genericFieldBorderOpTransactionFactory<TensorFieldData_t, PlusEqualsOp>maintains equivalent behavior while aligning with the operation-generalization pattern introduced across the codebase.src/amr/solvers/solver_ppc.hpp (1)
583-584: LGTM!The new
fillIonBorderscall is appropriately placed afterupdateIonscompletes, ensuring ion borders are filled with the final computed values. The placement follows the established messenger call pattern in this function.src/amr/messengers/hybrid_messenger.hpp (1)
359-362: LGTM!The new
fillIonBordersmethod follows the established delegation pattern and is consistent with the existingfillFluxBordersandfillDensityBordersmethods.src/amr/messengers/refiner.hpp (4)
6-7: LGTM!Headers updated to include the new operation-based transaction infrastructure and type utilities.
25-26: LGTM!New
RefinerTypeenum values for max-border operations extend the refiner capabilities appropriately.
37-38: LGTM!The operation type aliases provide clean parameterization for the transaction factories.
212-216: Good defensive programming.Adding the final
elsebranch that throws ensures compile-time exhaustiveness checking is supplemented with runtime safety for any unhandledRefinerTypevalues.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
1045-1057: LGTM!The new type aliases and member containers for max-refiner pools follow the established pattern used for sum-refiner pools. The naming is consistent and the initialization pattern matches.
250-254: LGTM!The level registration for max-refiners correctly mirrors the pattern used for sum-refiners above, ensuring proper schedule creation at each level.
879-894: Verify assertion consistency between this code andfillIonBordersfunction.The assertions at lines 881 and 888 check exact sizes before populating refiner pools. If
HybridMessengerInfo'smaxBorderFieldsandmaxBorderVecFieldssizes are guaranteed invariants, these assertions are reasonable defensive checks. However, confirm that both assertion sites (here and infillIonBorders) remain synchronized if field counts ever change in future updates.src/amr/data/field/field_data.hpp (4)
30-36: LGTM!Good restructuring to clarify public/private access for type aliases. The
value_typeis correctly public whileSetEqualOpis private as an implementation detail.
315-321: LGTM!The new templated
operate<Operation>andunpackStreamAnd<Operation>declarations provide a clean generic interface for different field operations (sum, max, equals). This aligns well with the corresponding changes inTensorFieldData.
443-461: LGTM!The out-of-class template definitions correctly delegate to the existing
copy_<Operation>andunpackStream<Operation>methods, maintaining consistency with the internal implementation while exposing the generic operation interface.
105-105: Good const-correctness improvement.Changing
const SAMRAI::hier::PatchData&toSAMRAI::hier::PatchData const&follows the east-const style consistently throughout the file.src/amr/messengers/field_operate_transaction.hpp (3)
17-21: LGTM!Good generalization from the sum-specific
FieldBorderSumTransactionto the genericFieldBorderOpTransaction<FieldData_t, Operation>. This allows reuse with different operations (sum, max, equals) through the template parameter.
149-161: LGTM!The
unpackStreammethod correctly uses the templatedunpackStreamAnd<Operation>to apply the generic operation during unpacking.
172-190: LGTM!The
copyLocalDatamethod correctly uses the templatedoperate<Operation>for local data operations, maintaining consistency with the remote (stream-based) path.src/amr/data/tensorfield/tensor_field_data.hpp (3)
49-53: LGTM!The restructuring to expose
value_typeas public while keepingSetEqualOpprivate mirrors the changes infield_data.hpp, maintaining consistency between the two field data implementations.
343-347: LGTM!The new templated method declarations match those in
FieldData, ensuring a consistent API across both field data types for the generic operation pattern.
489-509: LGTM!The out-of-class template definitions correctly implement the generic operation pattern:
unpackStreamAnd<Operation>delegates tounpackStream<Operation>operate<Operation>delegates tocopy_<Operation>This is consistent with the corresponding implementation in
FieldData.
| levelNbrs : limit working set of hierarchy levels to those requested, if scalar, returns just that level | ||
| time : the simulation time to access the appropriate data for the requested time | ||
| """ | ||
| from .hierarchy.hierarchy_utils import is_root_lvl # avoid cyclic imports |
Check notice
Code scanning / CodeQL
Cyclic import Note
| assert len(self.time_hier) == 1 | ||
|
|
||
| hier = (list(self.time_hier.values()))[0] | ||
| val = [0 for ilvl, lvl in hier.items()] |
There was a problem hiding this comment.
why taking 0 as the initial max ? I don't think this works if 0 is not part of the values or I miss the point of that function. Also, if this function intends to return the max per level for a quantity maybe this should lead to the refactoring of global_max()which would thus be the max(of that).
There was a problem hiding this comment.
the diff hierarchy takes the abs diff, but this should probably be max(int) * -1
if global_max is per level is it the global_max?
There was a problem hiding this comment.
two answers :
- the diff hierarchy takes the abs diff ok but this function is max, and could be used elsewhere so it should find the true max
- regarding global_max, I said it's finding the global max across all levels sure, but its code does the loop over each level to find the per level max and max that after. Your new max-per-level function duplicates that so that global max could just return the max of your max-per-level function
There was a problem hiding this comment.
updating PatchHierarchy.global_max to use the now free function per level, would create a cyclic import
and there is no corresponding min function yet
There was a problem hiding this comment.
frankly I don't think global_min/max should even be on the PatchHierarchy class
it makes no sense for particles, if also moved to a free function then that's probably simpler
There was a problem hiding this comment.
putting all the plotting stuff in a separate file which can use both hierarchy.py and hierarchy_utils.py is probably the way to go to avoid cyclic imports, global_min/max is only used by plotting
There was a problem hiding this comment.
agreed global max should not be for particle hierarchies, makes no sense
|
TODO hard code smallest patch size to avoid llnl/SAMRAI#311 |
nicolasaunai
left a comment
nicolasaunai
left a comment
There was a problem hiding this comment.
ok there was also to update the default smallest patch size to a temporary larger value than what the formula gives, commenting that it's tested manually to work for interp all but 1, and refer to llnl/SAMRAI#311
Also, the overlap diff hierarchy was originally intended to be used in field overlap tests which are atm inconvenient to debug, do we do that in there or in a separate one
This PR does not update tests to use the overlap diff hierarchy, the file the tests that currently do overlap checks could be updated, I propose later as this is kind of important already |
| there can be for each node existing in multiple patches's ghost boxes | ||
| """ | ||
| import pyphare.core.box as boxm | ||
| from pyphare.pharesee.geometry import hierarchy_overlaps |
Check notice
Code scanning / CodeQL
Cyclic import Note
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/core/box.py`:
- Around line 202-203: The current __setitem__ blindly does
self.__getitem__(box_or_slice)[:] = val which breaks for scalar/0-d selections;
change __setitem__ to call sel = self.__getitem__(box_or_slice) and if
isinstance(sel, Box) perform sel[:] = val, otherwise assign directly into the
backing ndarray (e.g., self._arr[box_or_slice] = val or self.array[box_or_slice]
= val) so non-Box assignments route to the underlying array while Box
assignments keep the existing behavior.
In `@pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py`:
- Around line 764-856: The helpers iterate patch.patch_datas blindly and call
FieldData-specific routines, which fails if a PatchHierarchy contains
ParticleData; update zero_patch_hierarchy_like, patch_hierarchy_has_non_zero,
and max_from_patch_hierarchy to either (A) only operate on FieldData entries
(e.g., check isinstance(pd, FieldData) before calling
zeros_like/has_non_zero/max_from and leave ParticleData untouched) or (B) add
explicit ParticleData handlers and extend the dispatch maps in
zeros_like/has_non_zero/max_from to include ParticleData-specific functions;
make the change consistently in zero_patch_hierarchy_like (when assigning
patch.patch_datas[key]), patch_hierarchy_has_non_zero (when checking pd), and
max_from_patch_hierarchy (when computing max_from(pd)) so ParticleData entries
no longer raise RuntimeError.
♻️ Duplicate comments (2)
pyphare/pyphare/pharesee/geometry.py (1)
442-443: Cyclic import detected by static analysis.While the local import pattern defers the import to runtime and prevents
ImportErrorduring module initialization, CodeQL still detects a structural import cycle. This is a known limitation of static analysis with lazy imports.If the cycle causes issues, consider restructuring by extracting
is_root_lvlto a lower-level utility module that neithergeometry.pynorhierarchy_utils.pydepends on cyclically.pyphare/pyphare/pharein/simulation.py (1)
408-413: Use the computed safety bound when hard-coding minimum patch sizes.The code computes
2*max_ghosts+1on line 409 to prevent ghost overlaps, but line 412 immediately replaces it with hard-coded[6, 9, 9]. While this works for current test cases (interp_order=1, max_ghosts=2), it breaks for higher interpolation orders. For example, with interp_order=5 (max_ghosts=6), the computed minimum would be 13, but the hard-coded 1D value is only 6. The validation on line 425 only checkssmallest_patch_size > max_ghosts, which does not enforce the full2*max_ghosts+1safety requirement. Take the maximum of both values to preserve the safety guarantee.Suggested fix
# to prevent primal ghost overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 - smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 + required_min = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 min_per_dim = [6, 9, 9] - smallest_patch_size = phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim) + smallest_patch_size = np.maximum( + required_min, phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim) + )
🧹 Nitpick comments (5)
pyphare/pyphare/core/box.py (1)
186-187: Add a Box type check (optional).
selectassumes aBoxand will throw attribute errors otherwise. A lightweightisinstance(box, Box)check with a clear message would make failures more diagnosable.pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
615-673: Optional: precompute patch lookups for overlap diffs
diff_patch_forlinearly scans patches for every overlap; on large hierarchies this can become a noticeable hotspot. Consider precomputing a per-level lookup (e.g., by patch id/box) to avoid repeated scans.Based on learnings, prioritizing minimal/efficient code.
src/amr/messengers/field_operate_transaction.hpp (1)
1-2: Header guard could mirror full filenameConsider aligning the guard with the full file name (
FIELD_OPERATE_TRANSACTION) for consistency.Based on learnings, prefer guards mirroring the full path/filename.♻️ Proposed guard rename
-#ifndef PHARE_AMR_MESSENGERS_FIELD_OP_TRANSACTION_HPP -#define PHARE_AMR_MESSENGERS_FIELD_OP_TRANSACTION_HPP +#ifndef PHARE_AMR_MESSENGERS_FIELD_OPERATE_TRANSACTION_HPP +#define PHARE_AMR_MESSENGERS_FIELD_OPERATE_TRANSACTION_HPP ... -#endif // PHARE_AMR_MESSENGERS_FIELD_OP_TRANSACTION_HPP +#endif // PHARE_AMR_MESSENGERS_FIELD_OPERATE_TRANSACTION_HPPAlso applies to: 246-246
src/amr/messengers/hybrid_messenger_info.hpp (1)
69-71: Clarify expected contents of max-border field lists.These vectors drive fixed-size assumptions elsewhere (e.g., 2 scalar, 1 vector). A brief comment on expected count/ordering would prevent misconfiguration.
🔎 Suggested doc tweak
- std::vector<std::string> maxBorderFields; - std::vector<std::string> maxBorderVecFields; + // max-border fills; expected sizes/order are assumed by HybridHybridMessengerStrategy + std::vector<std::string> maxBorderFields; + std::vector<std::string> maxBorderVecFields;src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
456-457: Don’t rely onassertfor runtime configuration validation.These size checks protect against out-of-range access and silent misconfiguration, but
assertis compiled out in release builds. Prefer an explicit runtime check (exception or PHARE_ASSERT-style macro) so failures are deterministic and diagnosable.✅ Example runtime checks
- assert(popDensityBorderSumRefiners_.size() % ions.size() == 0); + if (popDensityBorderSumRefiners_.size() % ions.size() != 0) + throw std::runtime_error( + "popDensityBorderSumRefiners_ size must be a multiple of ions.size()"); ... - assert(ionFluxBorderMaxRefiners_.size() == 1); - assert(ionDensityBorderMaxRefiners_.size() == 2); + if (ionFluxBorderMaxRefiners_.size() != 1 || ionDensityBorderMaxRefiners_.size() != 2) + throw std::runtime_error( + "ion*BorderMaxRefiners_ must be sized to 1 (flux) and 2 (density)"); ... - assert(info->maxBorderFields.size() == 2); // mass & charge densities + if (info->maxBorderFields.size() != 2) + throw std::runtime_error( + "maxBorderFields must contain exactly 2 scalar fields"); - assert(info->maxBorderVecFields.size() == 1); + if (info->maxBorderVecFields.size() != 1) + throw std::runtime_error( + "maxBorderVecFields must contain exactly 1 vector field");Also applies to: 490-493, 878-885
| def __setitem__(self, box_or_slice, val): | ||
| self.__getitem__(box_or_slice)[:] = val |
There was a problem hiding this comment.
Fix non-Box assignments in __setitem__.
self.__getitem__(box_or_slice)[:] = val fails for scalar/0‑d selections (e.g., DataSelector(a)[0] = v). Route non‑Box assignments directly to the backing array.
🔧 Proposed fix
def __setitem__(self, box_or_slice, val):
- self.__getitem__(box_or_slice)[:] = val
+ if isinstance(box_or_slice, Box):
+ self.__getitem__(box_or_slice)[:] = val
+ else:
+ self.data[box_or_slice] = val🤖 Prompt for AI Agents
In `@pyphare/pyphare/core/box.py` around lines 202 - 203, The current __setitem__
blindly does self.__getitem__(box_or_slice)[:] = val which breaks for scalar/0-d
selections; change __setitem__ to call sel = self.__getitem__(box_or_slice) and
if isinstance(sel, Box) perform sel[:] = val, otherwise assign directly into the
backing ndarray (e.g., self._arr[box_or_slice] = val or self.array[box_or_slice]
= val) so non-Box assignments route to the underlying array while Box
assignments keep the existing behavior.
| def zero_patch_hierarchy_like(hier, **kwargs): | ||
| from copy import deepcopy | ||
|
|
||
| times = phut.listify(kwargs.get("time", hier.times())) | ||
|
|
||
| cpy = deepcopy(hier) | ||
| cpy.time_hier = {} | ||
| for time in times: | ||
| cpy.time_hier[format_timestamp(time)] = deepcopy(hier.levels(time)) | ||
| for lvl in cpy.levels(time).values(): | ||
| for patch in lvl: | ||
| for key, pd in patch.patch_datas.items(): | ||
| patch.patch_datas[key] = zeros_like(pd) | ||
| assert not has_non_zero(cpy, time=time) | ||
| return cpy | ||
|
|
||
|
|
||
| def zero_field_data_like(field_data): | ||
| from copy import deepcopy | ||
|
|
||
| cpy = deepcopy(field_data) | ||
| assert id(cpy.dataset) == id(field_data.dataset) | ||
| cpy.dataset = np.zeros(field_data.dataset.shape) | ||
| assert id(cpy.dataset) != id(field_data.dataset) | ||
| return cpy | ||
|
|
||
|
|
||
| def zeros_like(that, **kwargs): | ||
| dispatch = { | ||
| PatchHierarchy: zero_patch_hierarchy_like, | ||
| FieldData: zero_field_data_like, | ||
| } | ||
| if type(that) not in dispatch: | ||
| raise RuntimeError( | ||
| "Cannot resolve type to zero_like, consider updating if required" | ||
| ) | ||
| return dispatch[type(that)](that, **kwargs) | ||
|
|
||
|
|
||
| def field_data_has_non_zero(field_data, **kwargs): | ||
| return bool(len(np.nonzero(field_data.dataset[:])[0])) | ||
|
|
||
|
|
||
| def patch_hierarchy_has_non_zero(hier, time, **kwargs): | ||
| for ilvl, lvl in hier.levels(time).items(): | ||
| for patch in lvl: | ||
| for pd in patch.patch_datas.values(): | ||
| if has_non_zero(pd): | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def has_non_zero(that, **kwargs): | ||
| dispatch = { | ||
| PatchHierarchy: patch_hierarchy_has_non_zero, | ||
| FieldData: field_data_has_non_zero, | ||
| } | ||
| if type(that) not in dispatch: | ||
| raise RuntimeError( | ||
| "Cannot resolve type to has_non_zero, consider updating if required" | ||
| ) | ||
| return dispatch[type(that)](that, **kwargs) | ||
|
|
||
|
|
||
| def max_from_field_data(field_data): | ||
| return np.max(field_data.dataset[:]) | ||
|
|
||
|
|
||
| def max_from_patch_hierarchy(hier, time, **kwargs): | ||
| qty = kwargs.get("qty") | ||
| vals = {} | ||
| for ilvl, lvl in hier.levels(time).items(): | ||
| for patch in lvl: | ||
| for key, pd in patch.patch_datas.items(): | ||
| if qty is None or key == qty: | ||
| if ilvl in vals: | ||
| vals[ilvl] = max(max_from(pd), vals[ilvl]) | ||
| else: | ||
| vals[ilvl] = max_from(pd) | ||
|
|
||
| return vals | ||
|
|
||
|
|
||
| def max_from(that, **kwargs): | ||
| dispatch = { | ||
| PatchHierarchy: max_from_patch_hierarchy, | ||
| FieldData: max_from_field_data, | ||
| } | ||
| if type(that) not in dispatch: | ||
| raise RuntimeError( | ||
| "Cannot resolve type to max_from, consider updating if required" | ||
| ) | ||
| return dispatch[type(that)](that, **kwargs) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find call sites for the helpers
rg -nP '\b(zeros_like|has_non_zero|max_from)\s*\('Repository: PHAREHUB/PHARE
Length of output: 6006
🏁 Script executed:
#!/bin/bash
# Check what 'differ' is in diff_diags.py
head -100 tools/diff_diags.py | tail -60Repository: PHAREHUB/PHARE
Length of output: 2398
🏁 Script executed:
#!/bin/bash
# Find the overlap_diff_hierarchy function definition
rg -A 20 'def overlap_diff_hierarchy'Repository: PHAREHUB/PHARE
Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Check what types of data can be stored in patch_datas
# Search for patch_datas usage and what types are stored
rg -B 3 -A 3 'patch_datas\[' pyphare/pyphare/pharesee/ | head -100Repository: PHAREHUB/PHARE
Length of output: 7304
🏁 Script executed:
#!/bin/bash
# Search for ParticleData class and its usage in hierarchy
rg -l 'class ParticleData'Repository: PHAREHUB/PHARE
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check the ParticleData class definition and how it's used
rg -A 10 'class ParticleData' pyphare/pyphare/pharesee/hierarchy/patchdata.pyRepository: PHAREHUB/PHARE
Length of output: 429
🏁 Script executed:
#!/bin/bash
# Check where patch_datas is populated and what types are stored
rg -B 3 -A 3 'patch_datas\[.*\] = ' pyphare/pyphare/pharesee/ | head -150Repository: PHAREHUB/PHARE
Length of output: 3620
🏁 Script executed:
#!/bin/bash
# Check GetE, GetB, GetNi methods to understand what hierarchies they return
rg -A 15 'def GetE|def GetB|def GetNi' pyphare/pyphare/pharesee/run/ | head -80Repository: PHAREHUB/PHARE
Length of output: 3389
🏁 Script executed:
#!/bin/bash
# Check if there are any cases where hierarchies with mixed field and particle data are created
rg -A 20 'def _get_hierarchy' pyphare/pyphare/pharesee/run/run.py | head -60Repository: PHAREHUB/PHARE
Length of output: 755
🏁 Script executed:
#!/bin/bash
# Check the hierarchy_from function to see what it creates
rg -A 30 'def hierarchy_from' pyphare/pyphare/pharesee/hierarchy/fromh5.py | head -80Repository: PHAREHUB/PHARE
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Check new_from_h5 to see what data is loaded
rg -A 40 'def new_from_h5' pyphare/pyphare/pharesee/hierarchy/fromh5.py | head -100Repository: PHAREHUB/PHARE
Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Check patch_levels_from_h5 to see what data gets loaded
rg -A 50 'def patch_levels_from_h5' pyphare/pyphare/pharesee/hierarchy/fromh5.py | head -120Repository: PHAREHUB/PHARE
Length of output: 1973
🏁 Script executed:
#!/bin/bash
# Check add_to_patchdata to see what types are created
rg -B 5 -A 50 'def add_to_patchdata' pyphare/pyphare/pharesee/hierarchy/fromh5.py | head -120Repository: PHAREHUB/PHARE
Length of output: 1827
Guard zero/has/max helpers against ParticleData hierarchies
zero_patch_hierarchy_like, patch_hierarchy_has_non_zero, and max_from_patch_hierarchy iterate through all entries in patch_datas but dispatch only for PatchHierarchy and FieldData. When hierarchies are loaded from h5 files, patch_datas can contain ParticleData in addition to FieldData. These helpers will raise RuntimeError at runtime if a hierarchy with ParticleData is passed. Consider filtering to only process FieldData or adding explicit ParticleData handling to avoid surprising failures.
🧰 Tools
🪛 Ruff (0.14.13)
797-799: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Unused function argument: kwargs
(ARG001)
807-807: Unused function argument: kwargs
(ARG001)
808-808: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
822-824: Avoid specifying long messages outside the exception class
(TRY003)
853-855: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py` around lines 764 -
856, The helpers iterate patch.patch_datas blindly and call FieldData-specific
routines, which fails if a PatchHierarchy contains ParticleData; update
zero_patch_hierarchy_like, patch_hierarchy_has_non_zero, and
max_from_patch_hierarchy to either (A) only operate on FieldData entries (e.g.,
check isinstance(pd, FieldData) before calling zeros_like/has_non_zero/max_from
and leave ParticleData untouched) or (B) add explicit ParticleData handlers and
extend the dispatch maps in zeros_like/has_non_zero/max_from to include
ParticleData-specific functions; make the change consistently in
zero_patch_hierarchy_like (when assigning patch.patch_datas[key]),
patch_hierarchy_has_non_zero (when checking pd), and max_from_patch_hierarchy
(when computing max_from(pd)) so ParticleData entries no longer raise
RuntimeError.
PhilipDeegan
changed the title
PhilipDeegan
force-pushed
the
hellman
branch
2 times, most recently
from
dcf5250 to
842d80a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyphare/pyphare/pharesee/geometry.py (1)
168-173:⚠️ Potential issue | 🟡 MinorUpdate the overlap docstring keys.
The docstring still lists 3 keys, but overlaps now include
"name". Please update the description to avoid confusion.
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py`:
- Around line 148-153: The loop currently uses
range(reference_hier.items().keys()) which will raise TypeError; change the
iteration to iterate the level keys directly (e.g., for ilvl in
reference_hier.levels(t).keys():) so ilvl is an actual level value you can pass
to PatchLevel and new_patches_from(compute, hierarchies, ilvl, t, **kwargs);
ensure you use the same ilvl symbol in the PatchLevel(...) call so behavior is
unchanged.
🧹 Nitpick comments (10)
pyphare/pyphare/core/box.py (1)
186-187: Rename ambiguous variablelto improve readability.The variable name
l(lowercase L) is easily confused with the digit1. Consider renaming toloorlowerfor clarity.♻️ Proposed fix
def select(data, box): - return data[tuple([slice(l, u + 1) for l, u in zip(box.lower, box.upper)])] + return data[tuple([slice(lo, hi + 1) for lo, hi in zip(box.lower, box.upper)])]pyphare/pyphare/pharein/simulation.py (1)
408-412: Dead code: Line 409 computation is always overridden.The
smallest_patch_sizecomputed at line 409 is immediately overridden at line 412 due to the SAMRAI workaround. Consider either commenting out line 409 or restructuring to make the intent clearer.♻️ Proposed fix to clarify the workaround
max_ghosts = get_max_ghosts() small_invalid_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) largest_patch_size = kwargs.get("largest_patch_size", None) - # to prevent primal ghost overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 - smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 + # NOTE: proper value would be: max_ghosts * 2 + 1 to prevent primal ghost overlaps min_per_dim = [6, 9, 9] smallest_patch_size = phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim)tools/diff_diags.py (4)
17-26: Move argument parsing inside__main__block.Lines 17-26 execute at module import time, which prevents importing this module for reuse or testing without providing command-line arguments. Move this initialization into the
__main__block.♻️ Proposed fix
+diag_dir = None +cpp = None +dpi = 300 +DO_PLOTS = True # global plot skip + +def main(): + global diag_dir, cpp + if len(sys.argv) == 1: + print("diag dir expected as first argument") + sys.exit(1) + diag_dir = sys.argv[1] + ph.NO_GUI() + cpp = cpp_lib() + startMPI() + check_diags() -if len(sys.argv) == 1: - print("diag dir expected as first argument") - sys.exit(1) - -diag_dir = sys.argv[1] - -ph.NO_GUI() -DO_PLOTS = True # global plot skip -cpp = cpp_lib() -dpi=300 # ... functions remain unchanged ... if __name__ == "__main__": - startMPI() - check_diags() + main()
38-38: Remove extraneousfprefix from f-strings without placeholders.Lines 38, 96, and 112 use f-strings but contain no placeholders. Use regular strings instead.
♻️ Proposed fix
- filename=plot_file_for_qty(plot_dir, f"ranks", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", time, f"L{ilvl}"),- filename=plot_file_for_qty(plot_dir, f"ionCharge", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionCharge", time, f"L{ilvl}"),- filename=plot_file_for_qty(plot_dir, f"ionMass", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionMass", time, f"L{ilvl}"),Also applies to: 96-96, 112-112
121-131: Improve exception handling to avoid swallowing errors.Catching bare
Exceptionand only printing it makes debugging difficult. Consider re-raising after logging, or at minimum, remove the unused variableeon line 125.♻️ Proposed fix
def check_diag(run, plot_dir, time, fn): try: fn(run, plot_dir, time) - - except FileNotFoundError as e: + except FileNotFoundError: print("File not found for", fn.__name__) - except Exception as e: + except Exception: import traceback - - print(f"Unknown Exception caught: \n{e}") print(traceback.format_exc()) + raise # Re-raise to avoid silently continuing
14-14: Consider moving test dependency or extracting shared code.Importing from
tests.simulator.test_advancein a tools script creates a dependency on test code. IfAdvanceTestBaseis needed here, consider extracting shared functionality to a common module.src/amr/messengers/field_operate_transaction.hpp (1)
176-182: Consider implementingprintClassDataor documenting the intentional throw.The
printClassDatamethod throws unconditionally. If this is intentional (e.g., debugging aid not yet implemented), consider adding a brief comment explaining why, or implement minimal output.♻️ Suggested documentation
template<typename FieldData_t, typename Operation> void FieldBorderOpTransaction<FieldData_t, Operation>::printClassData(std::ostream& stream) const { PHARE_LOG_SCOPE(2, "FieldBorderOpTransaction::printClassData"); - + // TODO: Implement debug output if needed for SAMRAI diagnostics throw std::runtime_error("FieldBorderOpTransaction::printClassData!"); }src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
876-891: Consider documenting the expected refiner counts.The assertions enforce that
maxBorderFieldshas exactly 2 elements (mass & charge densities) andmaxBorderVecFieldshas exactly 1 (flux). While correct, consider adding a brief comment explaining these invariants for maintainability.📝 Proposed comment addition
+ // Expects 2 max-border fields: ion mass density and ion charge density assert(info->maxBorderFields.size() == 2); // mass & charge densities for (auto const& field : info->maxBorderFields) ionDensityBorderMaxRefiners_.emplace_back(resourcesManager_) .addStaticRefiner( field, field, nullptr, field, std::make_shared<FieldGhostInterpOverlapFillPattern<GridLayoutT>>()); + // Expects 1 max-border vec field: ion flux assert(info->maxBorderVecFields.size() == 1);src/amr/data/field/field_data.hpp (1)
267-267: Minor inconsistency: buffer usesdoubleinstead ofvalue_type.The buffer declaration uses
std::vector<double>butvalue_typeis the appropriate type alias for consistency and type safety. This appears to be pre-existing but could be cleaned up.♻️ Suggested fix
- std::vector<double> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.); + std::vector<value_type> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.);pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
624-626: Consider refactoring to eliminate the geometry↔hierarchy_utils import cycle.While both modules currently handle the cycle with local imports inside functions (a safe pattern), moving
hierarchy_overlapsto a shared utility module or restructuring the dependency would improve code clarity and remove the need for local imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tools/diff_diags.py`:
- Line 51: Add a new function named min_max_patch_shape(differ, time) to
hierarchy_utils that computes and returns the minimum and maximum patch shapes
used by the differ at the given time; locate where hierarchy utilities are
defined (the module that should expose min_max_patch_shape) and implement the
function to inspect the differ's patch/grid data (use existing methods on the
differ object already used elsewhere in the module), compute min and max
dimensions (height/width/shape) across patches at the specified time, and return
them in the same tuple/format expected by tools/diff_diags.py (used by the print
call); ensure the function is exported from the module so callers like
min_max_patch_shape(differ, time) do not raise AttributeError.
🧹 Nitpick comments (9)
pyphare/pyphare/core/box.py (1)
186-187: Rename ambiguous variableltolower.The single-letter
lis visually indistinguishable from1in many fonts. Consider renaming for clarity.♻️ Proposed fix
def select(data, box): - return data[tuple([slice(l, u + 1) for l, u in zip(box.lower, box.upper)])] + return data[tuple([slice(lo, hi + 1) for lo, hi in zip(box.lower, box.upper)])]pyphare/pyphare/pharein/simulation.py (1)
408-412: Intentional dead assignment flagged by static analysis.The assignment on line 409 (
smallest_patch_size = ... * 2 + 1) is immediately overwritten on line 412, making it dead code. This is flagged by CodeQL. While the# TORMcomment indicates this is intentional and temporary pending resolution of SAMRAI issue#311, consider commenting out line 409 or removing it entirely to avoid static analysis noise and make the intent clearer.💡 Suggested clarification
# to prevent primal ghost overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 - smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 + # smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 min_per_dim = [6, 9, 9] smallest_patch_size = phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (3)
619-676: Cyclic import and assertion statements in production code.Two observations:
Cyclic import (line 621): The import of
pyphare.pharesee.geometryinside the function is noted by CodeQL. This is acceptable to break import cycles, but consider documenting this pattern.Assert statements (lines 669-670, 674): These assertions verify invariants but will be stripped in optimized Python (
python -O). If these checks are critical for correctness, consider using explicitifchecks withRuntimeError. If they're purely for development debugging, they're fine as-is.
785-792: Assertions verify implementation assumptions but may fail unexpectedly.The assertions on lines 789 and 791 verify that
deepcopyshares the dataset reference and that the new array is distinct. These are implementation detail checks that could break ifdeepcopybehavior changes or ifFieldDataimplements custom__deepcopy__. Consider removing these debug assertions or converting to explicit checks with informative error messages if they're meant to catch bugs.
807-817: Unusedkwargsand loop variable.Per static analysis:
kwargsis unused infield_data_has_non_zero(line 807) andpatch_hierarchy_has_non_zero(line 811)ilvlis unused in the loop on line 812♻️ Suggested cleanup
-def field_data_has_non_zero(field_data, **kwargs): +def field_data_has_non_zero(field_data, **_kwargs): return bool(len(np.nonzero(field_data.dataset[:])[0])) -def patch_hierarchy_has_non_zero(hier, time, **kwargs): - for ilvl, lvl in hier.levels(time).items(): +def patch_hierarchy_has_non_zero(hier, time, **_kwargs): + for _ilvl, lvl in hier.levels(time).items(): for patch in lvl:tools/diff_diags.py (4)
14-14: Unused import.
AdvanceTestBaseis imported but never used in this script. Remove the unused import.♻️ Suggested fix
-from tests.simulator.test_advance import AdvanceTestBase
38-38: Remove extraneousfprefix from strings without placeholders.Lines 38, 96, and 112 use f-strings but contain no placeholders. Remove the
fprefix for clarity.♻️ Suggested fixes
- filename=plot_file_for_qty(plot_dir, f"ranks", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", time, f"L{ilvl}"),- filename=plot_file_for_qty(plot_dir, f"ionCharge", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionCharge", time, f"L{ilvl}"),- filename=plot_file_for_qty(plot_dir, f"ionMass", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionMass", time, f"L{ilvl}"),Also applies to: 96-96, 112-112
121-131: Improve exception handling.Two minor issues:
- Line 125: The exception variable
eis assigned but unused. Either use it or remove the assignment.- Line 127: Catching broad
Exceptionis generally discouraged, though acceptable for a diagnostic tool that should be resilient.♻️ Suggested improvement
def check_diag(run, plot_dir, time, fn): try: fn(run, plot_dir, time) - except FileNotFoundError as e: + except FileNotFoundError: print("File not found for", fn.__name__) except Exception as e: import traceback print(f"Unknown Exception caught: \n{e}") print(traceback.format_exc())
89-91: Function namediff_current_densityis misleading.The function is named
diff_current_densitybut it processes ion charge density viarun.GetNi(time)and prints "ion charge rho max". Consider renaming todiff_charge_densityordiff_ion_densityfor clarity.
| print("Bx max: ", hootils.max_from(differ, time, qty="Bx")) | ||
| print("By max: ", hootils.max_from(differ, time, qty="By")) | ||
| print("Bz max: ", hootils.max_from(differ, time, qty="Bz")) | ||
| print("B patch shapes: ", hootils.min_max_patch_shape(differ, time)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for min_max_patch_shape definition across the repository
rg -n "def min_max_patch_shape" pyphare/Repository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the imports in tools/diff_diags.py and see the context around lines 51 and 73
head -80 tools/diff_diags.py | tail -n +1Repository: PHAREHUB/PHARE
Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Search for hootils import/alias in tools/diff_diags.py
rg -n "hootils\|hierarchy_utils\|import" tools/diff_diags.py | head -20Repository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
#!/bin/bash
# List all function definitions in hierarchy_utils.py
rg -n "^def " pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.pyRepository: PHAREHUB/PHARE
Length of output: 1784
🏁 Script executed:
#!/bin/bash
# Get file size and read hierarchy_utils.py
wc -l pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
cat -n pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.pyRepository: PHAREHUB/PHARE
Length of output: 33718
Add missing min_max_patch_shape function to hierarchy_utils.
The function min_max_patch_shape is called at lines 51 and 73 in tools/diff_diags.py but does not exist in pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py. This will cause an AttributeError at runtime. Implement this function in hierarchy_utils.py with the expected signature min_max_patch_shape(differ, time).
🤖 Prompt for AI Agents
In `@tools/diff_diags.py` at line 51, Add a new function named
min_max_patch_shape(differ, time) to hierarchy_utils that computes and returns
the minimum and maximum patch shapes used by the differ at the given time;
locate where hierarchy utilities are defined (the module that should expose
min_max_patch_shape) and implement the function to inspect the differ's
patch/grid data (use existing methods on the differ object already used
elsewhere in the module), compute min and max dimensions (height/width/shape)
across patches at the specified time, and return them in the same tuple/format
expected by tools/diff_diags.py (used by the print call); ensure the function is
exported from the module so callers like min_max_patch_shape(differ, time) do
not raise AttributeError.
| restart_options = kwargs.get("restart_options", {}) | ||
|
|
||
| if restart_options is not None: | ||
| if "restart_options" is kwargs: |
Check warning
Code scanning / CodeQL
Comparison using is when operands support `__eq__` Warning
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
148-154:⚠️ Potential issue | 🟡 MinorUse
reference_hier.levels(t).keys()to access levels for the current iteration time.Line 150 calls
reference_hier.items().keys()without the time parametert, even though it's inside afor t in reference_hier.times():loop. Thelevels()method is time-aware and takes an optional time parameter; the rest of the codebase useshier.levels(t).items()when iterating through times. Change toreference_hier.levels(t).keys()for consistency and correctness.pyphare/pyphare/pharein/simulation.py (1)
525-549:⚠️ Potential issue | 🔴 Critical
check_restart_optionscondition is always false — validation is never executed.Line 527:
if "restart_options" is kwargs:uses the identity operator (is) which compares a string to a dict object and always evaluates to false. This causes all validation logic to be skipped, including key validation, mode checking, directory validation, and restart_time resolution.Fix the condition
- if "restart_options" is kwargs: + if "restart_options" in kwargs:
🤖 Fix all issues with AI agents
In `@tools/diff_diags.py`:
- Around line 93-102: The ionCharge plotting block calls hootils.has_non_zero
and then always plots; change it to respect the global DO_PLOTS flag the same
way diff_mass_density does (i.e., only call differ.plot when DO_PLOTS is true)
and remove the unnecessary f-string around "ionCharge" by passing the plain
string to plot_file_for_qty; locate the block using hootils.has_non_zero(differ,
time), differ.levels(time), differ.plot(...) and plot_file_for_qty(...) to
implement the conditional and string fix.
🧹 Nitpick comments (10)
tools/diff_diags.py (4)
14-14: Remove unused import.
AdvanceTestBaseis imported but never used in this file.-from tests.simulator.test_advance import AdvanceTestBase
38-38: Remove extraneous f-prefix from string literal.
f"ranks"has no placeholders.- filename=plot_file_for_qty(plot_dir, f"ranks", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ranks", time, f"L{ilvl}"),
112-112: Remove extraneous f-prefix from string literal.
f"ionMass"has no placeholders.- filename=plot_file_for_qty(plot_dir, f"ionMass", time, f"L{ilvl}"), + filename=plot_file_for_qty(plot_dir, "ionMass", time, f"L{ilvl}"),
125-131: Clean up unused exception variable.The exception
eat line 125 is captured but never used.- except FileNotFoundError as e: + except FileNotFoundError: print("File not found for", fn.__name__)The broad
Exceptioncatch is reasonable for a diagnostic script that should continue processing other diagnostics.pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (5)
669-674: Replace assertions with explicit checks for production reliability.These
assertstatements (lines 669-670, 674) are used for correctness verification but will be silently skipped if Python is run with optimizations enabled (-Oflag). If these invariants must hold, convert them to explicitif/raisechecks.🛡️ Proposed fix
if len(np.nonzero(diff)[0]): boxm.DataSelector(diff_data0)[box_pd1] = np.maximum(dif0, diff) boxm.DataSelector(diff_data1)[box_pd2] = np.maximum(dif1, diff) - assert len(np.nonzero(diff_patch0.patch_datas[name].dataset)[0]) - assert len(np.nonzero(diff_patch1.patch_datas[name].dataset)[0]) + if not len(np.nonzero(diff_patch0.patch_datas[name].dataset)[0]): + raise RuntimeError("diff_patch0 should have non-zero values after update") + if not len(np.nonzero(diff_patch1.patch_datas[name].dataset)[0]): + raise RuntimeError("diff_patch1 should have non-zero values after update") found = 1 - if found: - assert has_non_zero(diff_hier, time=time) + if found and not has_non_zero(diff_hier, time=time): + raise RuntimeError("diff_hier should have non-zero values when diffs found")
768-782: Remove redundant import and consider explicit check.
deepcopyis already imported at the top of the file (line 2). The local import on line 769 is unnecessary. Also, theasserton line 781 has the same production reliability concern noted earlier.♻️ Proposed fix
def zero_patch_hierarchy_like(hier, **kwargs): - from copy import deepcopy - times = phut.listify(kwargs.get("time", hier.times())) cpy = deepcopy(hier) cpy.time_hier = {} for time in times: cpy.time_hier[format_timestamp(time)] = deepcopy(hier.levels(time)) for lvl in cpy.levels(time).values(): for patch in lvl: for key, pd in patch.patch_datas.items(): patch.patch_datas[key] = zeros_like(pd) - assert not has_non_zero(cpy, time=time) + if has_non_zero(cpy, time=time): + raise RuntimeError("Zeroed hierarchy should not have non-zero values") return cpy
785-792: Remove debug assertions and redundant import.The assertions on lines 789 and 791 test implementation details of
deepcopybehavior rather than functional correctness. Line 789 asserts thatdeepcopydoes not copy the dataset (same id), which is unusual and relies onFieldData's specific__deepcopy__implementation. These checks are fragile and read like debug/test code.Also,
deepcopyis already imported at the module level.♻️ Proposed fix
def zero_field_data_like(field_data): - from copy import deepcopy - cpy = deepcopy(field_data) - assert id(cpy.dataset) == id(field_data.dataset) cpy.dataset = np.zeros(field_data.dataset.shape) - assert id(cpy.dataset) != id(field_data.dataset) return cpy
807-808: Unusedkwargsparameter.The
kwargsparameter is unused. If it's kept for dispatch signature consistency, consider renaming to**_kwargsor**_to signal intent and silence the linter warning.
811-817: Minor: unusedkwargsand loop variable.The
kwargsparameter is unused, andilvlis not used within the loop body. Consider renaming to signal intent.♻️ Proposed fix
-def patch_hierarchy_has_non_zero(hier, time, **kwargs): - for ilvl, lvl in hier.levels(time).items(): +def patch_hierarchy_has_non_zero(hier, time, **_kwargs): + for _ilvl, lvl in hier.levels(time).items(): for patch in lvl: for pd in patch.patch_datas.values(): if has_non_zero(pd): return True return Falsepyphare/pyphare/pharein/simulation.py (1)
408-412: Consider preserving ghost-overlap safety when hard-codingsmallest_patch_size.Right now the
max_ghosts * 2 + 1requirement is overwritten unconditionally. A safer approach is to keep the hard-coded minimum but also enforce the computed minimum.Proposed refinement
- smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 + computed_min = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 min_per_dim = [6, 9, 9] - smallest_patch_size = phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim) + hard_min = phare_utilities.np_array_ify(min_per_dim[ndim - 1], ndim) + smallest_patch_size = np.maximum(computed_min, hard_min)
| if hootils.has_non_zero(differ, time): | ||
| for ilvl in differ.levels(time).keys(): | ||
| differ.plot( | ||
| filename=plot_file_for_qty(plot_dir, f"ionCharge", time, f"L{ilvl}"), | ||
| plot_patches=True, | ||
| vmin=0, | ||
| vmax=+1e-16, | ||
| levels=(ilvl,), | ||
| dpi=dpi, | ||
| ) |
There was a problem hiding this comment.
Inconsistent DO_PLOTS check and unnecessary f-prefix.
diff_current_density plots unconditionally when non-zero, while diff_mass_density (line 109) respects DO_PLOTS. Also, f"ionCharge" has no placeholders.
Suggested fix
- if hootils.has_non_zero(differ, time):
+ if DO_PLOTS and hootils.has_non_zero(differ, time):
for ilvl in differ.levels(time).keys():
differ.plot(
- filename=plot_file_for_qty(plot_dir, f"ionCharge", time, f"L{ilvl}"),
+ filename=plot_file_for_qty(plot_dir, "ionCharge", time, f"L{ilvl}"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hootils.has_non_zero(differ, time): | |
| for ilvl in differ.levels(time).keys(): | |
| differ.plot( | |
| filename=plot_file_for_qty(plot_dir, f"ionCharge", time, f"L{ilvl}"), | |
| plot_patches=True, | |
| vmin=0, | |
| vmax=+1e-16, | |
| levels=(ilvl,), | |
| dpi=dpi, | |
| ) | |
| if DO_PLOTS and hootils.has_non_zero(differ, time): | |
| for ilvl in differ.levels(time).keys(): | |
| differ.plot( | |
| filename=plot_file_for_qty(plot_dir, "ionCharge", time, f"L{ilvl}"), | |
| plot_patches=True, | |
| vmin=0, | |
| vmax=+1e-16, | |
| levels=(ilvl,), | |
| dpi=dpi, | |
| ) |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@tools/diff_diags.py` around lines 93 - 102, The ionCharge plotting block
calls hootils.has_non_zero and then always plots; change it to respect the
global DO_PLOTS flag the same way diff_mass_density does (i.e., only call
differ.plot when DO_PLOTS is true) and remove the unnecessary f-string around
"ionCharge" by passing the plain string to plot_file_for_qty; locate the block
using hootils.has_non_zero(differ, time), differ.levels(time), differ.plot(...)
and plot_file_for_qty(...) to implement the conditional and string fix.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyphare/pyphare/pharein/simulation.py (1)
525-549:⚠️ Potential issue | 🟠 MajorUse
ininstead ofisfor membership check.The identity check
"restart_options" is kwargsalways evaluates to False (a string literal is never the same object as a dictionary), so the entire validation block never executes. This leaves invalid modes/keys undetected and skipsrestart_timenormalization.🔧 Suggested fix
- if "restart_options" is kwargs: + if "restart_options" in kwargs:pyphare/pyphare/pharein/__init__.py (1)
362-368:⚠️ Potential issue | 🟡 MinorGuard
restart_timevalue, not just key presence.
While the normal initialization path ensuresrestart_timeis not None before being added torestart_options, users could explicitly pass{"restart_time": None}, which would cause the C++ binding to fail. Use a value check instead.🔧 Suggested fix
- if "restart_time" in restart_options: + if restart_options.get("restart_time") is not None: from pyphare.cpp import cpp_etc_lib
🤖 Fix all issues with AI agents
In `@tools/diff_diags.py`:
- Around line 29-30: The f-string in plot_file_for_qty is invalid because it
nests a string literal inside the f-expression; replace the nested format
expression with a proper f-string formatting of the time variable (e.g., use
time:.10f inside the f-string) or build the formatted time separately and
interpolate it; update the function plot_file_for_qty to produce the same
filename pattern "harris_t{time:.10f}_{qty}_{extra}.png" without nested quotes.
| def plot_file_for_qty(plot_dir, qty, time, extra=""): | ||
| return f"{plot_dir}/harris_t{"{:.10f}".format(time)}_{qty}_{extra}.png" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd tools/diff_diags.pyRepository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
find . -name "diff_diags.py" -type fRepository: PHAREHUB/PHARE
Length of output: 79
🏁 Script executed:
rg "plot_file_for_qty" --type pyRepository: PHAREHUB/PHARE
Length of output: 2540
🏁 Script executed:
head -40 tools/diff_diags.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 1321
🏁 Script executed:
python3 << 'EOF'
import ast
code = '''def plot_file_for_qty(plot_dir, qty, time, extra=""):
return f"{plot_dir}/harris_t{"{:.10f}".format(time)}_{qty}_{extra}.png"
'''
try:
ast.parse(code)
print("✓ Code parses successfully (NO SYNTAX ERROR)")
except SyntaxError as e:
print(f"✗ SYNTAX ERROR: {e}")
print(f" Line: {e.lineno}, Offset: {e.offset}")
EOFRepository: PHAREHUB/PHARE
Length of output: 139
🏁 Script executed:
python3 << 'EOF'
import ast
fix = '''def plot_file_for_qty(plot_dir, qty, time, extra=""):
return f"{plot_dir}/harris_t{time:.10f}_{qty}_{extra}.png"
'''
try:
ast.parse(fix)
print("✓ Suggested fix parses successfully")
except SyntaxError as e:
print(f"✗ SYNTAX ERROR in fix: {e}")
EOFRepository: PHAREHUB/PHARE
Length of output: 93
Fix f-string formatting in plot_file_for_qty (syntax error).
The nested double quotes in the f-string expression cause an invalid decimal literal error and prevent the code from parsing.
🔧 Suggested fix
-def plot_file_for_qty(plot_dir, qty, time, extra=""):
- return f"{plot_dir}/harris_t{"{:.10f}".format(time)}_{qty}_{extra}.png"
+def plot_file_for_qty(plot_dir, qty, time, extra=""):
+ return f"{plot_dir}/harris_t{time:.10f}_{qty}_{extra}.png"🤖 Prompt for AI Agents
In `@tools/diff_diags.py` around lines 29 - 30, The f-string in plot_file_for_qty
is invalid because it nests a string literal inside the f-expression; replace
the nested format expression with a proper f-string formatting of the time
variable (e.g., use time:.10f inside the f-string) or build the formatted time
separately and interpolate it; update the function plot_file_for_qty to produce
the same filename pattern "harris_t{time:.10f}_{qty}_{extra}.png" without nested
quotes.
2 participants
output from new harris diff test