Vtk hdf #1084
Vtk hdf #1084
Conversation
📝 WalkthroughWalkthroughAdds a VTK-HDF5 diagnostic writer and format selection, exposes pyphare.tovtk, updates diagnostic model view and HDF5 helpers, refactors several core utilities and types, removes Python init scripts, and adds MPI-enabled tests for VTK diagnostic output. Changes
Sequence Diagram(s)sequenceDiagram
participant Sim as Simulation
participant DM as DiagnosticsManagerResolver
participant Writer as DiagnosticsWriterFactory
participant H5 as h5::H5Writer
participant VTK as vtkh5::H5Writer
participant TW as TypeWriter
Sim->>DM: request diagnostics manager (dict with "format")
DM->>Writer: make_unique(hier, model, dict)
Writer->>Writer: read dict["format"]
alt format == "phareh5"
Writer->>H5: instantiate h5::H5Writer<ModelView>
H5-->>Sim: return h5 writer
else format == "pharevtkhdf"
Writer->>VTK: instantiate vtkh5::H5Writer<ModelView>
VTK->>VTK: build typeWriters_ (info,meta,fluid,electromag,particle)
VTK-->>Sim: return vtk writer
else unknown
Writer->>Sim: throw runtime_error
end
Sim->>H5: dump(diagnostics, timestamp)
Sim->>VTK: dump(diagnostics, timestamp)
VTK->>TW: for each diagnostic: getDiagnosticWriterForType -> write(diagnostic)
TW->>VTK: use VTKFileWriter to iterate levels, init datasets, write fields/tensors (MPI offsets)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
PhilipDeegan
force-pushed
the
vtk
branch
2 times, most recently
from
39d7491 to
0c9462d
Compare
| template<typename Writer> | ||
| class H5TypeWriter : public PHARE::diagnostic::TypeWriter | ||
| { | ||
| using FloatType = float; // Writer::FloatType; |
Check notice
Code scanning / CodeQL
Commented-out code Note
There was a problem hiding this comment.
the vtk reader in paraview doesn't seem to support doubles
| auto const& layout // | ||
| ) | ||
| { | ||
| bool static constexpr c_ordering = false; |
Check notice
Code scanning / CodeQL
Unused local variable Note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/python3/patch_data.hpp (1)
35-36: Consider passing string parameters by const reference.The
patchIDandoriginparameters insetPatchData,setPatchDataFromGrid, andsetPatchDataFromFieldare passed by value, creating unnecessary copies. Since these functions don't require ownership, passing them asstd::string const&would be more efficient.Apply this pattern to avoid copies:
-void setPatchData(PatchData& data, std::string const patchID, std::string const origin, +void setPatchData(PatchData& data, std::string const& patchID, std::string const& origin, Container lower, Container upper)-void setPatchDataFromGrid(PatchData& pdata, GridLayout& grid, std::string patchID) +void setPatchDataFromGrid(PatchData& pdata, GridLayout& grid, std::string const& patchID)-void setPatchDataFromField(PatchData& pdata, Field const& field, GridLayout& grid, - std::string patchID) +void setPatchDataFromField(PatchData& pdata, Field const& field, GridLayout& grid, + std::string const& patchID)Also applies to: 46-46, 54-55
src/core/data/ndarray/ndarray_vector.hpp (1)
38-38: Simplify unused parameter.The inline comment
/*nCells*/for the unused parameter is non-standard. Modern C++ convention is to simply omit the parameter name.Apply this diff:
- static inline std::uint32_t idx(auto const /*nCells*/, Idx i) { return i; } + static inline std::uint32_t idx(auto const, Idx i) { return i; }src/amr/resources_manager/amr_utils.hpp (1)
247-247: Preserve perfect forwarding into resman.setOnPatchNow that
visitHierarchyforwardsargs, we should propagate the perfect forwarding down intosetOnPatch; otherwise rvalues (e.g., move-only helpers) devolve to lvalues when named insidevisitLevel, preventing those call-sites from compiling or forcing extra copies. Applyingstd::forwardhere keeps the chain intact.- auto guard = resman.setOnPatch(*patch, args...); + auto guard = resman.setOnPatch(*patch, std::forward<Args>(args)...);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
pyphare/pyphare/pharein/__init__.py(1 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)pyphare/pyphare/pharesee/__init__.py(1 hunks)pyphare/pyphare/pharesee/tovtk.py(1 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(3 hunks)src/core/data/grid/gridlayoutimplyee.hpp(1 hunks)src/core/data/ndarray/ndarray_vector.hpp(5 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/algorithm.hpp(2 hunks)src/core/utilities/box/box.hpp(2 hunks)src/core/utilities/mpi_utils.hpp(1 hunks)src/core/utilities/types.hpp(3 hunks)src/diagnostic/detail/h5writer.hpp(1 hunks)src/diagnostic/detail/vtk_types/electromag.hpp(1 hunks)src/diagnostic/detail/vtk_types/fluid.hpp(1 hunks)src/diagnostic/detail/vtk_types/info.hpp(1 hunks)src/diagnostic/detail/vtk_types/meta.hpp(1 hunks)src/diagnostic/detail/vtk_types/particle.hpp(1 hunks)src/diagnostic/detail/vtkh5_type_writer.hpp(1 hunks)src/diagnostic/detail/vtkh5_writer.hpp(1 hunks)src/diagnostic/diagnostic_manager.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(6 hunks)src/diagnostic/diagnostics.hpp(2 hunks)src/hdf5/detail/h5/h5_file.hpp(2 hunks)src/phare/CMakeLists.txt(0 hunks)src/phare/phare_init.py(0 hunks)src/phare/phare_init_small.py(0 hunks)src/python3/patch_data.hpp(1 hunks)tests/simulator/CMakeLists.txt(1 hunks)tests/simulator/__init__.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/test_vtk_diagnostics.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/phare/CMakeLists.txt
- src/phare/phare_init_small.py
- src/phare/phare_init.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/diagnostic/diagnostics.hppsrc/python3/patch_data.hppsrc/amr/physical_models/hybrid_model.hppsrc/amr/resources_manager/amr_utils.hppsrc/diagnostic/detail/vtk_types/particle.hppsrc/diagnostic/detail/vtk_types/fluid.hppsrc/core/utilities/box/box.hppsrc/core/utilities/mpi_utils.hppsrc/diagnostic/detail/h5writer.hppsrc/diagnostic/detail/vtk_types/info.hppsrc/core/data/grid/gridlayout.hppsrc/diagnostic/diagnostic_manager.hppsrc/core/data/grid/gridlayoutimplyee.hppsrc/core/utilities/types.hppsrc/core/data/ndarray/ndarray_vector.hppsrc/hdf5/detail/h5/h5_file.hppsrc/diagnostic/detail/vtk_types/meta.hppsrc/core/utilities/algorithm.hppsrc/diagnostic/diagnostic_model_view.hppsrc/diagnostic/detail/vtkh5_writer.hppsrc/core/data/tensorfield/tensorfield.hppsrc/diagnostic/detail/vtk_types/electromag.hppsrc/diagnostic/detail/vtkh5_type_writer.hpp
🧠 Learnings (7)
📓 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-02-07T14:35:14.630Z
Learnt from: nicolasaunai
Repo: PHAREHUB/PHARE PR: 591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.
Applied to files:
tests/simulator/CMakeLists.txt
📚 Learning: 2025-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/amr/CMakeLists.txt:103-105
Timestamp: 2025-07-09T17:18:05.771Z
Learning: In the PHARE project, `HighFive` is a CMake option defined in `res/cmake/options.cmake` as `option(HighFive "Build with highfive usage" ON)`, not a target that would be created by find_package(). The condition `if (HighFive)` correctly checks this option.
Applied to files:
src/diagnostic/detail/h5writer.hpp
📚 Learning: 2024-07-26T22:04:34.160Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 0
File: :0-0
Timestamp: 2024-07-26T22:04:34.160Z
Learning: The variable `c_ordering` in `ConcreteLoadBalancerHybridStrategyNPPC<PHARE_T>::compute` is used to create an array view `lb_view` for handling load balancing data.
Applied to files:
src/core/utilities/algorithm.hpp
📚 Learning: 2025-09-17T13:35:11.533Z
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.
Applied to files:
src/core/utilities/algorithm.hpp
📚 Learning: 2024-09-09T13:57:02.285Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Applied to files:
src/diagnostic/detail/vtkh5_writer.hppsrc/diagnostic/detail/vtkh5_type_writer.hpp
📚 Learning: 2025-09-17T13:50:16.038Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/tensorfield/tensor_field_data_factory.hpp:112-112
Timestamp: 2025-09-17T13:50:16.038Z
Learning: PhilipDeegan indicated that getSizeOfMemory returning a placeholder value of 1 in TensorFieldDataFactory is acceptable for their use case, suggesting it may not be critical for PHARE's memory management needs.
Applied to files:
src/core/data/tensorfield/tensorfield.hpp
🧬 Code graph analysis (20)
src/diagnostic/diagnostics.hpp (3)
src/diagnostic/diagnostic_manager.hpp (5)
dict(83-83)dict(86-86)dict(86-86)make_unique(74-80)make_unique(74-75)src/diagnostic/detail/vtkh5_writer.hpp (2)
make_unique(67-74)make_unique(67-67)src/diagnostic/detail/h5writer.hpp (2)
make_unique(61-68)make_unique(61-61)
src/amr/resources_manager/amr_utils.hpp (1)
src/core/data/field/field.hpp (4)
args(75-81)args(75-75)args(83-86)args(83-83)
src/diagnostic/detail/vtk_types/particle.hpp (4)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtkh5_type_writer.hpp (4)
diagnostic(87-92)diagnostic(87-87)ilvl(385-414)ilvl(385-385)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/diagnostic/detail/h5writer.hpp (4)
diagnostic(94-98)diagnostic(94-94)iLevel(210-214)iLevel(210-210)
src/diagnostic/detail/vtk_types/fluid.hpp (3)
src/diagnostic/detail/vtkh5_type_writer.hpp (6)
diagnostic(87-92)diagnostic(87-87)ilvl(385-414)ilvl(385-385)boxes(57-71)boxes(57-57)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)
src/core/utilities/box/box.hpp (1)
src/core/utilities/types.hpp (2)
product(265-268)product(265-265)
tests/simulator/refinement/test_2d_10_core.py (2)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)tests/simulator/test_advance.py (1)
AdvanceTestBase(33-788)
src/diagnostic/detail/vtk_types/info.hpp (7)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtk_types/meta.hpp (2)
write(29-31)write(29-29)src/diagnostic/detail/vtk_types/particle.hpp (2)
write(31-54)write(31-31)src/diagnostic/detail/vtkh5_type_writer.hpp (2)
diagnostic(87-92)diagnostic(87-87)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/diagnostic/detail/h5writer.hpp (2)
diagnostic(94-98)diagnostic(94-94)
src/core/data/grid/gridlayout.hpp (2)
src/core/utilities/types.hpp (5)
auto(151-156)auto(491-494)auto(497-500)for_N_make_array(484-487)for_N_make_array(484-484)src/core/utilities/box/box.hpp (6)
grow(88-93)grow(88-88)grow(323-328)grow(323-323)i(96-96)i(96-96)
tests/simulator/test_vtk_diagnostics.py (7)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/simulator/simulator.py (2)
startMPI(40-44)reset(254-265)tests/simulator/__init__.py (4)
SimulatorTest(201-267)tearDown(204-205)unique_diag_dir_for_test_case(250-254)run(246-248)tests/diagnostic/__init__.py (1)
dump_all_diags(15-61)pyphare/pyphare/pharein/simulation.py (2)
simulation_domain(978-979)Simulation(745-1080)pyphare/pyphare/pharein/maxwellian_fluid_model.py (1)
MaxwellianFluidModel(8-303)pyphare/pyphare/pharesee/tovtk.py (1)
convert(269-416)
tests/simulator/__init__.py (1)
pyphare/pyphare/core/box.py (1)
Box(5-69)
src/core/data/grid/gridlayoutimplyee.hpp (1)
tests/core/data/gridlayout/test_linear_combinaisons_yee.py (1)
dualToPrimal(27-35)
src/core/utilities/types.hpp (1)
src/core/data/tensorfield/tensorfield.hpp (2)
for_N(83-87)for_N(88-92)
src/core/data/ndarray/ndarray_vector.hpp (1)
src/core/utilities/types.hpp (2)
product(265-268)product(265-265)
src/diagnostic/detail/vtk_types/meta.hpp (5)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtk_types/info.hpp (2)
write(31-33)write(31-31)src/diagnostic/detail/vtkh5_type_writer.hpp (2)
diagnostic(87-92)diagnostic(87-87)src/diagnostic/detail/types/meta.hpp (1)
PHARE(7-143)
pyphare/pyphare/pharesee/tovtk.py (1)
src/diagnostic/detail/vtkh5_type_writer.hpp (8)
shape(114-121)shape(114-114)shape(123-131)shape(123-123)ilvl(385-414)ilvl(385-385)resize(417-423)resize(417-417)
src/core/utilities/algorithm.hpp (2)
src/core/utilities/types.hpp (2)
all(355-358)all(355-355)src/core/data/ndarray/ndarray_vector.hpp (4)
make_array_view(214-217)make_array_view(214-214)make_array_view(220-223)make_array_view(220-220)
src/diagnostic/diagnostic_model_view.hpp (5)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)
patch(137-145)patch(137-137)ions(410-420)ions(410-411)ions(424-450)ions(424-425)ions(452-486)ions(452-453)ions(498-537)ions(498-499)ions(545-551)ions(545-546)src/amr/resources_manager/amr_utils.hpp (2)
visitHierarchy(257-266)visitHierarchy(257-258)src/core/data/electrons/electrons.hpp (16)
layout(107-133)layout(107-107)layout(285-285)layout(285-285)layout(286-286)layout(286-286)layout(309-319)layout(309-309)forward_as_tuple(47-50)forward_as_tuple(52-55)forward_as_tuple(181-184)forward_as_tuple(186-186)forward_as_tuple(254-257)forward_as_tuple(259-262)forward_as_tuple(330-333)forward_as_tuple(335-338)src/core/data/ions/ion_population/ion_population.hpp (1)
forward_as_tuple(109-113)src/core/data/ions/ions.hpp (1)
forward_as_tuple(225-229)
src/diagnostic/detail/vtkh5_writer.hpp (4)
src/diagnostic/diagnostic_model_view.hpp (1)
dimension(30-61)src/diagnostic/detail/h5writer.hpp (15)
H5Writer(51-56)H5Writer(58-58)H5Writer(194-194)H5Writer(195-195)make_unique(61-68)make_unique(61-61)level(72-73)timestamp(101-104)timestamp(101-101)diagnostic(94-98)diagnostic(94-94)modelView_(160-160)timestamp_(161-161)dump(224-250)dump(224-225)src/diagnostic/diagnostics.hpp (2)
make_unique(50-65)make_unique(50-51)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
level(309-331)level(309-309)level(333-337)level(333-333)diagnostic(87-92)diagnostic(87-87)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
diagnostic(87-92)diagnostic(87-87)ilvl(385-414)ilvl(385-385)boxes(57-71)boxes(57-57)
src/diagnostic/detail/vtkh5_type_writer.hpp (7)
src/core/utilities/types.hpp (5)
get_env_as(249-249)sum_from(277-285)sum_from(277-277)sum(271-274)sum(271-271)src/hdf5/detail/h5/h5_file.hpp (9)
data(66-66)s(248-248)s(248-248)s(249-253)s(249-249)path(267-273)path(267-267)path(275-280)path(275-275)src/core/utilities/box/box.hpp (11)
box(50-50)i(96-96)i(96-96)grow(88-93)grow(88-88)grow(323-328)grow(323-323)s(209-234)s(209-209)s(235-240)s(235-235)src/core/data/tensorfield/tensorfield.hpp (6)
i(178-178)i(178-178)i(179-179)i(179-179)tensor_field_dim_from_rank(18-24)tensor_field_dim_from_rank(18-18)src/core/utilities/mpi_utils.hpp (4)
rank(25-25)collect(17-17)collect(214-229)collect(214-214)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/core/utilities/algorithm.hpp (2)
convert_to_fortran_primal(137-140)convert_to_fortran_primal(137-137)
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/tovtk.py
[notice] 163-163: Unused local variable
Variable cellData_g is not used.
[notice] 165-165: Unused local variable
Variable fieldData_g is not used.
[notice] 166-166: Unused local variable
Variable cellDataOffset_g is not used.
[notice] 167-167: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
[notice] 168-168: Unused local variable
Variable FieldDataOffset_g is not used.
[failure] 361-361: Wrong number of arguments in a call
Call to function primalScalarToFlatPrimal with too many arguments; should be no more than 5.
[failure] 387-387: Wrong number of arguments in a call
Call to function BtoFlatPrimal with too few arguments; should be no fewer than 6.
Call to function EtoFlatPrimal with too few arguments; should be no fewer than 6.
Call to function primalVectorToFlatPrimal with too few arguments; should be no fewer than 6.
src/core/utilities/algorithm.hpp
[notice] 105-105: Unused local variable
Variable c_ordering is not used.
src/diagnostic/detail/vtkh5_type_writer.hpp
[notice] 41-41: Commented-out code
This comment appears to contain commented-out code.
🪛 Ruff (0.14.2)
tests/simulator/test_vtk_diagnostics.py
1-1: Shebang is present but file is not executable
(EXE001)
27-27: Unused function argument: xyz
(ARG001)
34-34: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
41-41: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
44-44: Unused function argument: xyz
(ARG001)
47-47: Unused function argument: xyz
(ARG001)
54-54: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
61-61: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
64-64: Unused function argument: xyz
(ARG001)
67-67: Unused function argument: xyz
(ARG001)
70-70: Unused function argument: xyz
(ARG001)
121-121: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
pyphare/pyphare/pharesee/tovtk.py
1-1: Shebang is present but file is not executable
(EXE001)
17-19: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
263-263: Consider [*root_spacing, 0, 0] instead of concatenation
Replace with [*root_spacing, 0, 0]
(RUF005)
265-265: Consider [*root_spacing, 0] instead of concatenation
Replace with [*root_spacing, 0]
(RUF005)
321-321: Local variable cellData_g is assigned to but never used
Remove assignment to unused variable cellData_g
(F841)
323-323: Local variable fieldData_g is assigned to but never used
Remove assignment to unused variable fieldData_g
(F841)
324-324: Local variable cellDataOffset_g is assigned to but never used
Remove assignment to unused variable cellDataOffset_g
(F841)
326-326: Local variable FieldDataOffset_g is assigned to but never used
Remove assignment to unused variable FieldDataOffset_g
(F841)
384-384: Prefer next(iter(patch.keys())) over single element slice
Replace with next(iter(patch.keys()))
(RUF015)
🔇 Additional comments (8)
src/python3/patch_data.hpp (1)
26-30: Critical fix: Correct variadic template forwarding.The change from
std::forward<Args...>(args...)tostd::forward<Args>(args)...fixes invalid syntax. The ellipsis must appear outside thestd::forwardcall to properly expand each argument in the parameter pack individually, ensuring correct perfect forwarding semantics.src/hdf5/detail/h5/h5_file.hpp (1)
98-99: LGTM!The defensive existence check prevents duplicate dataset creation and returns the existing dataset if found.
src/core/utilities/types.hpp (2)
248-254: Verify error handling for invalid environment variable values.This function depends on
from_string<T>, which silently returns a default-initialized value on conversion failure. If an environment variable exists but contains an invalid value for typeT, this function will return an unexpected result without any indication of failure.Ensure that upstream callers can handle or validate the returned values, especially for critical configuration parameters.
483-487: LGTM!This is a clean convenience wrapper around
for_Nwith themake_arraymode. The implementation properly forwards the function parameter and maintains constexpr correctness.src/core/utilities/mpi_utils.hpp (1)
5-7: LGTM!The added includes are necessary and properly support the file's dependencies:
phare_mpi.hppfor MPI definitions used throughoutspan.hppforSpanSettemplate (lines 169, 174, 204)types.hppfor type traits likeis_std_vector_v,is_std_array_v, anddisplacementFrom(lines 77, 154, 221)src/core/data/ndarray/ndarray_vector.hpp (3)
55-70: LGTM!The unified index computation via
idx(...)followed by bounds-checked access throughdata[i]provides a clean, consistent access pattern across all dimensionalities.
101-101: LGTM! Consistent template parameter updates.All usage sites have been correctly updated to use
NdArrayViewerwithout theDataTypetemplate parameter, maintaining consistency with the simplified template signature.Also applies to: 138-138, 271-271, 283-283
22-23: Reconsider index type constraint—no evidence of practical limitation.The
std::uint16_tindex type appears to be an intentional design constraint for this physics simulation codebase. Shape values are stored asstd::uint32_tarrays (lines 124, 209, 307), but indices are constrained touint16_t—a deliberate trade-off likely for memory efficiency or simulation requirements. No codebase search found array dimensions exceeding 65K in any single dimension, and type promotion in index calculations prevents overflow. This design aligns with typical physics grid constraints for plasma simulations.
| auto AMRBoxFor(auto const& field) const | ||
| { | ||
| auto const centerings = centering(field); | ||
| return grow(AMRGhostBoxFor(field), for_N_make_array<dimension>([&](auto i) { | ||
| return -1 * nbrGhosts(centerings[i]); | ||
| })); |
There was a problem hiding this comment.
Fix unsigned wraparound in AMRBoxFor shrink
nbrGhosts returns std::uint32_t, so -1 * nbrGhosts(...) is still unsigned and wraps to a huge positive number. The call to grow(...) therefore expands the box dramatically instead of shrinking it back to the physical extent. Please cast to a signed type before applying the minus, e.g.:
- return grow(AMRGhostBoxFor(field), for_N_make_array<dimension>([&](auto i) {
- return -1 * nbrGhosts(centerings[i]);
- }));
+ return grow(AMRGhostBoxFor(field), for_N_make_array<dimension>([&](auto i) {
+ return -static_cast<int>(nbrGhosts(centerings[i]));
+ }));📝 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.
| auto AMRBoxFor(auto const& field) const | |
| { | |
| auto const centerings = centering(field); | |
| return grow(AMRGhostBoxFor(field), for_N_make_array<dimension>([&](auto i) { | |
| return -1 * nbrGhosts(centerings[i]); | |
| })); | |
| auto AMRBoxFor(auto const& field) const | |
| { | |
| auto const centerings = centering(field); | |
| return grow(AMRGhostBoxFor(field), for_N_make_array<dimension>([&](auto i) { | |
| return -static_cast<int>(nbrGhosts(centerings[i])); | |
| })); | |
| } |
🤖 Prompt for AI Agents
In src/core/data/grid/gridlayout.hpp around lines 1205 to 1210, the expression
-1 * nbrGhosts(...) performs unsigned arithmetic because nbrGhosts returns
std::uint32_t, causing wraparound; cast nbrGhosts(...) to a signed integer type
before negation (e.g. static_cast<std::int32_t>(nbrGhosts(centerings[i])) or a
wider signed type if needed) so the subtraction yields a negative signed value,
then pass that signed value to grow(...); ensure the cast happens inside the
lambda and keep the surrounding template usage unchanged.
| template<typename T> | ||
| NO_DISCARD T from_string(std::string const& s) | ||
| { | ||
| T t; | ||
| std::stringstream ss(s); | ||
| ss >> t; | ||
| return t; | ||
| } |
There was a problem hiding this comment.
Add missing include and error handling.
The function has the following issues:
- Missing include:
std::stringstreamrequires<sstream>, which is not included in this file. - Silent failure: If the stream extraction (
ss >> t) fails, the function returns a default-initialized or partially-initialized value with no indication of failure.
Apply this diff to add the missing include at the top of the file:
#include <cassert>
#include <array>
#include <iomanip>
#include <optional>
+#include <sstream>
#include <string>For error handling, consider one of these approaches:
- Return
std::optional<T>to signal conversion failures - Add stream state checks and throw an exception on failure
- Document that callers must only use this with types that have well-defined default values for invalid input
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/utilities/types.hpp around lines 224 to 231, add the missing include
for <sstream> (and <optional>) at the top of the file and replace the current
from_string<T> to return std::optional<T>; inside the function construct
std::stringstream from s, attempt extraction, check ss.good()/!ss.fail() (or
operator bool) and return std::optional<T>{t} on success or std::nullopt on
failure; update the function signature and document that callers must handle
std::nullopt (or adjust call sites accordingly).
| auto const write_quantity = [&](auto& layout, auto const&, auto const iLevel) { | ||
| // | ||
| }; | ||
|
|
||
| modelView.onLevels( | ||
| [&](auto const& lvl) { | ||
| auto const ilvl = lvl.getLevelNumber(); | ||
| initializer.initFileLevel(ilvl); | ||
|
|
||
| // auto boxes = modelView.localLevelBoxes(ilvl); | ||
| // for (auto* vecField : this->h5Writer_.modelView().getElectromagFields()) | ||
| // if (diagnostic.quantity == "/" + vecField->name()) | ||
| // initializer.template initTensorFieldFileLevel<1>(ilvl, boxes); | ||
|
|
||
| modelView.visitHierarchy(write_quantity, ilvl, ilvl); | ||
| }, | ||
| this->h5Writer_.minLevel, this->h5Writer_.maxLevel); |
There was a problem hiding this comment.
Particle diagnostics never write data
write_quantity is empty, so modelView.visitHierarchy does nothing and the particle VTK HDF output files stay empty. That breaks particle diagnostics as soon as this writer is selected. Please port the actual write logic (e.g., mirroring the existing HDF5 ParticlesDiagnosticWriter) so each population/layout emits its datasets before closing the file.
| for (auto* diagnostic : diagnostics) | ||
| if (!file_flags.count(diagnostic->type + diagnostic->quantity)) | ||
| file_flags[diagnostic->type + diagnostic->quantity] = this->flags; | ||
|
|
||
| for (auto* diagnostic : diagnostics) | ||
| typeWriters_.at(diagnostic->type)->write(*diagnostic); | ||
| } |
There was a problem hiding this comment.
Restore post-write flag downgrade
Line 181 keeps file_flags set to whatever came from this->flags. In overwrite mode that includes HiFile::Truncate, so every subsequent dump reopens the file in truncate mode and erases earlier time steps. Please reset the entry back to READ_WRITE once the write completes, as done before this refactor.
Apply this diff:
for (auto* diagnostic : diagnostics)
- typeWriters_.at(diagnostic->type)->write(*diagnostic);
+ typeWriters_.at(diagnostic->type)->write(*diagnostic);
+
+ for (auto* diagnostic : diagnostics)
+ file_flags[diagnostic->type + diagnostic->quantity] = READ_WRITE;🤖 Prompt for AI Agents
In src/diagnostic/detail/vtkh5_writer.hpp around lines 177 to 183, after setting
file_flags[diagnostic->type + diagnostic->quantity] = this->flags and calling
the writer, the code fails to reset that entry back to HiFile::READ_WRITE,
leaving Truncate (overwrite) set for subsequent dumps; restore the previous
behavior by assigning file_flags[diagnostic->type + diagnostic->quantity] =
HiFile::READ_WRITE after the write completes so subsequent opens do not truncate
earlier time steps.
| template<typename Type> | ||
| auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace) | ||
| { | ||
| if (exist(path)) | ||
| return h5file_.getDataSet(path); | ||
|
|
||
| HighFive::DataSetCreateProps props; | ||
| props.add(HighFive::Chunking{chunk}); | ||
| return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props); | ||
| } |
There was a problem hiding this comment.
Missing parent group creation and consider explicit parameter types.
Two concerns:
-
Unlike
create_data_set(Line 100), this function doesn't callcreateGroupsToDataSet(path)before creating the dataset. If parent groups don't exist,h5file_.createDataSetwill fail. -
The
autoparameters require C++20 abbreviated function templates. Consider explicit template parameters for clarity and broader compatibility.
Apply this diff to add parent group creation:
template<typename Type>
auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace)
{
if (exist(path))
return h5file_.getDataSet(path);
+ createGroupsToDataSet(path);
HighFive::DataSetCreateProps props;
props.add(HighFive::Chunking{chunk});
return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props);
}📝 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.
| template<typename Type> | |
| auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace) | |
| { | |
| if (exist(path)) | |
| return h5file_.getDataSet(path); | |
| HighFive::DataSetCreateProps props; | |
| props.add(HighFive::Chunking{chunk}); | |
| return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props); | |
| } | |
| template<typename Type> | |
| auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace) | |
| { | |
| if (exist(path)) | |
| return h5file_.getDataSet(path); | |
| createGroupsToDataSet(path); | |
| HighFive::DataSetCreateProps props; | |
| props.add(HighFive::Chunking{chunk}); | |
| return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props); | |
| } |
🤖 Prompt for AI Agents
In src/hdf5/detail/h5/h5_file.hpp around lines 105 to 114, the
create_chunked_data_set template is missing the call to
createGroupsToDataSet(path) before creating the dataset and is using abbreviated
function-template parameters; update it to mirror create_data_set by first
checking/creating parent groups (call createGroupsToDataSet(path) when the path
doesn't exist) and replace the auto parameters with explicit types (e.g., use a
string-like const& for path, an appropriate container/array type const& for
chunk sizes, and the concrete dataspace type) so the dataset creation won't fail
when parent groups are absent and the function is clearer/compatible with
pre-C++20 code.
| auto getDataSet(std::string const& s) | ||
| { | ||
| assert(exist(s)); | ||
| return h5file_.getDataSet(s); | ||
| } |
There was a problem hiding this comment.
Replace assertion with exception for safer error handling.
The assert(exist(s)) is compiled out in release builds (when NDEBUG is defined), potentially causing undefined behavior if a non-existent dataset is accessed. Consider throwing an exception for production safety.
Apply this diff:
auto getDataSet(std::string const& s)
{
- assert(exist(s));
+ if (!exist(s))
+ throw std::runtime_error("Dataset does not exist: " + s);
return h5file_.getDataSet(s);
}📝 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.
| auto getDataSet(std::string const& s) | |
| { | |
| assert(exist(s)); | |
| return h5file_.getDataSet(s); | |
| } | |
| auto getDataSet(std::string const& s) | |
| { | |
| if (!exist(s)) | |
| throw std::runtime_error("Dataset does not exist: " + s); | |
| return h5file_.getDataSet(s); | |
| } |
🤖 Prompt for AI Agents
In src/hdf5/detail/h5/h5_file.hpp around lines 249 to 253, replace the
assert(exist(s)) with a runtime check that throws a descriptive exception (e.g.,
std::runtime_error or a project-specific exception type) when the dataset does
not exist, and ensure the exception message includes the dataset name and
context; also add the necessary header include for the chosen exception type if
not present. Maintain the existing return by calling and returning
h5file_.getDataSet(s) after the check.
PhilipDeegan
force-pushed
the
vtk
branch
4 times, most recently
from
9371cab to
da3b967
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
src/hdf5/detail/h5/h5_file.hpp (3)
105-114: Missing parent group creation will cause runtime failure.Unlike
create_data_set(line 100), this function doesn't callcreateGroupsToDataSet(path)before creating the dataset. If parent groups don't exist,h5file_.createDataSetwill fail at runtime.Apply this diff to add parent group creation:
template<typename Type> auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace) { if (exist(path)) return h5file_.getDataSet(path); + createGroupsToDataSet(path); HighFive::DataSetCreateProps props; props.add(HighFive::Chunking{chunk}); return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props); }
249-253: Replace assertion with exception for production safety.The
assert(exist(s))is compiled out in release builds (whenNDEBUGis defined), leading to undefined behavior if a non-existent dataset is accessed. This could cause crashes or silent data corruption in production.Apply this diff:
auto getDataSet(std::string const& s) { - assert(exist(s)); + if (!exist(s)) + throw std::runtime_error("Dataset does not exist: " + s); return h5file_.getDataSet(s); }
255-258: Fix incorrect special member function signatures.Two critical issues:
- Line 256: Move constructor declared with
const&&defeats move semantics since you cannot transfer ownership from a const object.- Line 257: Copy assignment operator missing
constqualifier on parameter, which is non-standard and could allow accidental modification of the source object.Apply this diff to correct both signatures:
HighFiveFile(HighFiveFile const&) = delete; -HighFiveFile(HighFiveFile const&&) = delete; -HighFiveFile& operator=(HighFiveFile&) = delete; +HighFiveFile(HighFiveFile&&) = delete; +HighFiveFile& operator=(HighFiveFile const&) = delete; HighFiveFile& operator=(HighFiveFile&&) = delete;src/core/utilities/types.hpp (1)
224-231: Missing include and error handling already flagged.The issues with missing
<sstream>include and silent failure on conversion errors have already been identified in a previous review.pyphare/pyphare/pharesee/tovtk.py (1)
80-91: Correct 3D By primalization averaging.The 3D branch reuses
ph_bzand averages along the Y axis, so By mixes the wrong component and skips the X-direction dual pair. The exported magnetic field is therefore incorrect.Replace the averaging with this fix:
- by[:, :, :] = 0.5 * ( - 0.5 - * ( - ph_by[domain, domainP1, domain][:, 1:, :] - + ph_by[domain, domainP1, domain][:, :-1, :] - ) - + 0.5 - * ( - ph_bz[domain, domain, domainP1][:, :, 1:] - + ph_bz[domain, domain, domainP1][:, :, :-1] - ) - ) + by[:, :, :] = 0.5 * ( + 0.5 + * ( + ph_by[domainP1, domain, domain][1:, :, :] + + ph_by[domainP1, domain, domain][:-1, :, :] + ) + + 0.5 + * ( + ph_by[domain, domain, domainP1][:, :, 1:] + + ph_by[domain, domain, domainP1][:, :, :-1] + ) + )src/diagnostic/detail/vtk_types/particle.hpp (1)
37-54: Particle VTK writer is still a no-op
write_quantitynever writes anything, sovisitHierarchyproduces empty particle datasets and every particle diagnostic is silently lost. This is the same blocker that was raised earlier—please port the actual write logic (e.g., iterate populations/layouts and feed the initializer just like the existing H5 ParticlesDiagnosticWriter does) before landing this format.src/diagnostic/detail/vtkh5_writer.hpp (1)
177-183: Restore post-write flag downgrade to prevent data loss.Line 179 sets
file_flags[diagnostic->type + diagnostic->quantity] = this->flags, which in overwrite mode includesHiFile::Truncate. This flag persists across subsequentdump()calls, causing each dump to reopen the file in truncate mode and erase earlier time steps.Apply this diff to reset flags after writing:
for (auto* diagnostic : diagnostics) typeWriters_.at(diagnostic->type)->write(*diagnostic); + + for (auto* diagnostic : diagnostics) + file_flags[diagnostic->type + diagnostic->quantity] = READ_WRITE;
🧹 Nitpick comments (6)
src/amr/resources_manager/amr_utils.hpp (1)
229-239: Consider forwarding args for consistency withvisitHierarchy.While
visitHierarchynow properly forwards the variadic arguments (line 251),visitLevelreceives forwarding references but passes them tosetOnPatchwithout forwarding (line 234). For consistency and to preserve move semantics, consider applying the same forwarding pattern.Apply this diff to forward the arguments:
template<typename GridLayout, typename ResMan, typename Action, typename... Args> void visitLevel(SAMRAI_Types::level_t& level, ResMan& resman, Action&& action, Args&&... args) { for (auto& patch : level) { - auto guard = resman.setOnPatch(*patch, args...); + auto guard = resman.setOnPatch(*patch, std::forward<Args>(args)...); GridLayout layout = layoutFromPatch<GridLayout>(*patch); action(layout, to_string(patch->getGlobalId()), static_cast<std::size_t>(level.getLevelNumber())); } }tests/simulator/refinement/test_2d_10_core.py (1)
17-24: Consider refactoring for test isolation.Module-level initialization of
cpp,test,L0_diags, andL0L1_diagscreates global mutable state that persists across imports. While this works for single-use test scripts, it could impact test isolation if this module is later integrated into a larger test suite.Consider lazy initialization or moving these to
main()to improve test isolation and initialization order control.Example refactor moving initialization to main():
-cpp = cpp_lib() -test = AdvanceTestBase(rethrow=True) # change to False for debugging images -L0_diags = "phare_outputs/test_x_homo_0" -L0L1_diags = "phare_outputs/test_x_homo_1" - ph.NO_GUI() +# Module-level variables to be initialized in main() +cpp = None +test = None +L0_diags = "phare_outputs/test_x_homo_0" +L0L1_diags = "phare_outputs/test_x_homo_1" def config(diag_outputs, model_init={}, refinement_boxes=None):Then initialize in
main():def main(): + global cpp, test import random startMPI() + cpp = cpp_lib() + test = AdvanceTestBase(rethrow=True) rando = random.randint(0, int(1e10))tests/simulator/test_vtk_diagnostics.py (2)
34-34: Consider replacing lambda assignments with def statements.PEP 8 recommends using
defstatements instead of assigning lambdas to variables for better traceability and debugging.Example refactor for one occurrence:
- _ = lambda i: 0.1 * np.sin(2 * np.pi * xyz[i] / L[i]) + def component_value(i): + return 0.1 * np.sin(2 * np.pi * xyz[i] / L[i])Also applies to: 41-41, 54-54, 61-61
169-169: Clarify the status of EM_B conversion.The commented-out line suggests
tovtkconversion fails forEM_B.h5.Should this be tracked as a TODO/FIXME, or is this a known limitation that's acceptable for this PR? If it's a bug, consider opening an issue to track it.
Do you want me to help create an issue for tracking the EM_B conversion problem?
src/diagnostic/detail/vtkh5_writer.hpp (1)
130-130: Hard-coded maxLevel limit.The hard-coded
maxLevel = 10may be insufficient for simulations requiring deeper AMR hierarchies.Consider making this configurable via the
PHAREDictinmake_unique, e.g.:auto maxLevel = cppdict::get_value(dict, "max_level", std::size_t{10});src/diagnostic/detail/vtkh5_type_writer.hpp (1)
361-361: Document the X_TIMES multiplier logic.The dimension-dependent multiplier
X_TIMES(4 for 1D, 2 for 2D, 1 for 3D) implements an important conversion factor but lacks explanation.Add a comment explaining why the multiplier is needed:
// VTK stores data per cube face: 2D data requires 2 faces, 1D requires 4 faces, 3D is 1:1 constexpr static auto X_TIMES = std::array{4, 2, /* 3d noop */ 1}[dimension - 1];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
pyphare/pyphare/pharein/__init__.py(1 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)pyphare/pyphare/pharesee/__init__.py(1 hunks)pyphare/pyphare/pharesee/tovtk.py(1 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(3 hunks)src/core/data/grid/gridlayoutdefs.hpp(1 hunks)src/core/data/grid/gridlayoutimplyee.hpp(27 hunks)src/core/data/ndarray/ndarray_vector.hpp(5 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/algorithm.hpp(2 hunks)src/core/utilities/box/box.hpp(2 hunks)src/core/utilities/types.hpp(3 hunks)src/diagnostic/detail/h5writer.hpp(1 hunks)src/diagnostic/detail/vtk_types/electromag.hpp(1 hunks)src/diagnostic/detail/vtk_types/fluid.hpp(1 hunks)src/diagnostic/detail/vtk_types/info.hpp(1 hunks)src/diagnostic/detail/vtk_types/meta.hpp(1 hunks)src/diagnostic/detail/vtk_types/particle.hpp(1 hunks)src/diagnostic/detail/vtkh5_type_writer.hpp(1 hunks)src/diagnostic/detail/vtkh5_writer.hpp(1 hunks)src/diagnostic/diagnostic_manager.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(7 hunks)src/diagnostic/diagnostics.hpp(2 hunks)src/hdf5/detail/h5/h5_file.hpp(2 hunks)src/phare/CMakeLists.txt(0 hunks)src/phare/phare_init.py(0 hunks)src/phare/phare_init_small.py(0 hunks)tests/simulator/CMakeLists.txt(2 hunks)tests/simulator/__init__.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/test_vtk_diagnostics.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/phare/CMakeLists.txt
- src/phare/phare_init_small.py
- src/phare/phare_init.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/diagnostic/diagnostic_manager.hpp
- pyphare/pyphare/pharein/simulation.py
- tests/simulator/init.py
- pyphare/pyphare/pharein/init.py
- src/core/data/grid/gridlayout.hpp
- pyphare/pyphare/pharesee/init.py
- src/diagnostic/detail/vtk_types/meta.hpp
- src/diagnostic/detail/h5writer.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/core/utilities/algorithm.hpp
- tests/simulator/CMakeLists.txt
- src/core/utilities/box/box.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/core/data/grid/gridlayoutdefs.hppsrc/core/data/tensorfield/tensorfield.hppsrc/amr/resources_manager/amr_utils.hppsrc/diagnostic/detail/vtk_types/electromag.hppsrc/diagnostic/detail/vtk_types/particle.hppsrc/diagnostic/detail/vtk_types/fluid.hppsrc/diagnostic/diagnostics.hppsrc/core/utilities/types.hppsrc/hdf5/detail/h5/h5_file.hppsrc/core/data/grid/gridlayoutimplyee.hppsrc/core/data/ndarray/ndarray_vector.hppsrc/diagnostic/detail/vtkh5_writer.hppsrc/diagnostic/detail/vtk_types/info.hppsrc/diagnostic/diagnostic_model_view.hppsrc/diagnostic/detail/vtkh5_type_writer.hpp
🧠 Learnings (2)
📓 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-09-09T13:57:02.285Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Applied to files:
src/diagnostic/detail/vtkh5_writer.hppsrc/diagnostic/detail/vtkh5_type_writer.hpp
🧬 Code graph analysis (13)
src/amr/resources_manager/amr_utils.hpp (1)
src/core/data/field/field.hpp (4)
args(75-81)args(75-75)args(83-86)args(83-83)
src/diagnostic/detail/vtk_types/electromag.hpp (3)
src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (8)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)boxes(57-71)boxes(57-57)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)
tests/simulator/test_vtk_diagnostics.py (5)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/simulator/simulator.py (2)
startMPI(41-45)reset(258-269)tests/simulator/__init__.py (3)
SimulatorTest(201-267)tearDown(204-205)unique_diag_dir_for_test_case(250-254)tests/diagnostic/__init__.py (1)
dump_all_diags(15-62)pyphare/pyphare/pharesee/tovtk.py (1)
convert(269-416)
src/diagnostic/detail/vtk_types/particle.hpp (3)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)
src/diagnostic/detail/vtk_types/fluid.hpp (2)
src/diagnostic/detail/vtkh5_type_writer.hpp (8)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)boxes(57-71)boxes(57-57)src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)
src/diagnostic/diagnostics.hpp (3)
src/diagnostic/diagnostic_manager.hpp (5)
dict(83-83)dict(86-86)dict(86-86)make_unique(74-80)make_unique(74-75)src/diagnostic/detail/vtkh5_writer.hpp (2)
make_unique(67-74)make_unique(67-67)src/diagnostic/detail/h5writer.hpp (2)
make_unique(61-68)make_unique(61-61)
src/core/data/ndarray/ndarray_vector.hpp (1)
src/core/utilities/types.hpp (2)
product(265-268)product(265-265)
tests/simulator/refinement/test_2d_10_core.py (2)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)tests/simulator/test_advance.py (1)
AdvanceTestBase(33-869)
src/diagnostic/detail/vtkh5_writer.hpp (4)
src/diagnostic/diagnostic_model_view.hpp (1)
dimension(30-60)src/diagnostic/diagnostics.hpp (2)
make_unique(50-65)make_unique(50-51)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
level(298-334)level(298-298)level(336-340)level(336-336)diagnostic(87-92)diagnostic(87-87)src/hdf5/detail/h5/h5_file.hpp (4)
path(267-273)path(267-267)path(275-280)path(275-275)
src/diagnostic/detail/vtk_types/info.hpp (5)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtk_types/meta.hpp (2)
write(29-31)write(29-29)src/diagnostic/detail/vtk_types/particle.hpp (2)
write(31-54)write(31-31)src/diagnostic/detail/vtkh5_type_writer.hpp (2)
diagnostic(87-92)diagnostic(87-87)
src/diagnostic/diagnostic_model_view.hpp (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (10)
patch(146-154)patch(146-146)ions(407-416)ions(407-407)ions(420-445)ions(420-420)ions(447-480)ions(447-447)ions(492-531)ions(492-493)src/amr/resources_manager/amr_utils.hpp (2)
visitHierarchy(244-253)visitHierarchy(244-245)src/core/data/ions/ion_population/ion_population.hpp (1)
forward_as_tuple(109-113)src/core/data/ions/ions.hpp (1)
forward_as_tuple(225-229)
src/diagnostic/detail/vtkh5_type_writer.hpp (5)
src/core/utilities/types.hpp (7)
get_env_as(249-249)for_N_make_array(504-507)for_N_make_array(504-504)sum_from(277-285)sum_from(277-277)sum(271-274)sum(271-271)src/core/utilities/box/box.hpp (7)
box(51-51)i(97-97)i(97-97)grow(89-94)grow(89-89)grow(275-280)grow(275-275)src/core/data/tensorfield/tensorfield.hpp (6)
i(181-181)i(181-181)i(182-182)i(182-182)tensor_field_dim_from_rank(17-24)tensor_field_dim_from_rank(17-17)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/core/utilities/algorithm.hpp (2)
convert_to_fortran_primal(137-140)convert_to_fortran_primal(137-137)
pyphare/pyphare/pharesee/tovtk.py (2)
pyphare/pyphare/core/box.py (1)
shape(32-34)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
ilvl(388-417)ilvl(388-388)lvl(267-272)lvl(267-267)resize(420-426)resize(420-420)
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/tovtk.py
[failure] 361-361: Wrong number of arguments in a call
Call to function primalScalarToFlatPrimal with too many arguments; should be no more than 5.
[failure] 387-387: Wrong number of arguments in a call
Call to function BtoFlatPrimal with too few arguments; should be no fewer than 6.
Call to function EtoFlatPrimal with too few arguments; should be no fewer than 6.
Call to function primalVectorToFlatPrimal with too few arguments; should be no fewer than 6.
[notice] 163-163: Unused local variable
Variable cellData_g is not used.
[notice] 165-165: Unused local variable
Variable fieldData_g is not used.
[notice] 166-166: Unused local variable
Variable cellDataOffset_g is not used.
[notice] 167-167: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
[notice] 168-168: Unused local variable
Variable FieldDataOffset_g is not used.
🪛 Ruff (0.14.3)
tests/simulator/test_vtk_diagnostics.py
1-1: Shebang is present but file is not executable
(EXE001)
27-27: Unused function argument: xyz
(ARG001)
34-34: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
41-41: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
44-44: Unused function argument: xyz
(ARG001)
47-47: Unused function argument: xyz
(ARG001)
54-54: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
61-61: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
64-64: Unused function argument: xyz
(ARG001)
67-67: Unused function argument: xyz
(ARG001)
70-70: Unused function argument: xyz
(ARG001)
121-121: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
pyphare/pyphare/pharesee/tovtk.py
1-1: Shebang is present but file is not executable
(EXE001)
17-19: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
263-263: Consider [*root_spacing, 0, 0] instead of concatenation
Replace with [*root_spacing, 0, 0]
(RUF005)
265-265: Consider [*root_spacing, 0] instead of concatenation
Replace with [*root_spacing, 0]
(RUF005)
321-321: Local variable cellData_g is assigned to but never used
Remove assignment to unused variable cellData_g
(F841)
323-323: Local variable fieldData_g is assigned to but never used
Remove assignment to unused variable fieldData_g
(F841)
324-324: Local variable cellDataOffset_g is assigned to but never used
Remove assignment to unused variable cellDataOffset_g
(F841)
326-326: Local variable FieldDataOffset_g is assigned to but never used
Remove assignment to unused variable FieldDataOffset_g
(F841)
384-384: Prefer next(iter(patch.keys())) over single element slice
Replace with next(iter(patch.keys()))
(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). (7)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (4)
src/hdf5/detail/h5/h5_file.hpp (1)
98-99: LGTM! Good defensive check.The existence check makes dataset creation idempotent, allowing safe repeated calls.
src/amr/resources_manager/amr_utils.hpp (1)
251-251: LGTM! Critical correctness fix for perfect forwarding.The corrected forwarding syntax
std::forward<Args>(args)...properly expands the parameter pack, forwarding each argument individually with its original value category. This matches the pattern shown in the relevant code snippets fromfield.hpp.tests/simulator/refinement/test_2d_10_core.py (1)
20-23: Initialization order with MPI startup is a valid concern—verify actual runtime behavior.
cpp_lib()is imported at module load time (line 20), but MPI initialization viaSamraiLifeCycle()doesn't occur untilstartMPI()is called inmain()(line 188) or viaSimulatorTest.setUp(). The C++ bindings module (pybindlibs.cpp) may require MPI context to be initialized at import time. Additionally, instantiatingtest = AdvanceTestBase(rethrow=True)at module level (line 21) before MPI is set up could cause issues if the class initialization has MPI dependencies, thoughsetUp()is only called by the test framework before test methods run.Consider moving module-level initialization inside
main()afterstartMPI()is called, or implement lazy initialization (defercpp_lib()andAdvanceTestBaseinstantiation until after MPI setup). This is especially important for MPI-based tests that initialize at module scope.src/diagnostic/detail/vtkh5_type_writer.hpp (1)
138-139: Verify duplicate write() calls in 2D writers.Both
VTKFileFieldWriter::write2DandVTKFileTensorFieldWriter::write2Dcallwrite()twice consecutively without any intervening state change. This pattern is unusual and may indicate either:
- An intentional handling of 2D data peculiarities
- A copy-paste error
Please confirm whether the duplicate
write()calls are intentional. If intentional, add a comment explaining why 2D data requires two identical write passes.If this is an error, remove the duplicate call:
}; write(); - write(); }Also applies to: 187-188
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0, 0, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{0, iShift, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{0, 0, iShift}, 0.25}; | ||
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{iShift, iShift, 0}, 0.25}; | ||
| return std::array{P1, P2, P3, P4}; |
There was a problem hiding this comment.
Fix 3D Bx→moment offsets.
For Bx (primal in X, dual in Y & Z) the 3D averaging must cover all Y/Z dual combinations. Point P4 currently shifts X instead of Z, so we never sample the (0, iShift, iShift) corner and the weights no longer sum over the correct stencil—magnetic flux is wrong.
Apply this diff to fix the offsets:
- constexpr WeightPoint<dimension> P4{Point<int, dimension>{iShift, iShift, 0}, 0.25};
+ constexpr WeightPoint<dimension> P4{Point<int, dimension>{0, iShift, iShift}, 0.25};📝 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.
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0, 0, 0}, 0.25}; | |
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{0, iShift, 0}, 0.25}; | |
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{0, 0, iShift}, 0.25}; | |
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{iShift, iShift, 0}, 0.25}; | |
| return std::array{P1, P2, P3, P4}; | |
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0, 0, 0}, 0.25}; | |
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{0, iShift, 0}, 0.25}; | |
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{0, 0, iShift}, 0.25}; | |
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{0, iShift, iShift}, 0.25}; | |
| return std::array{P1, P2, P3, P4}; |
🤖 Prompt for AI Agents
In src/core/data/grid/gridlayoutimplyee.hpp around lines 531-535, P4 currently
shifts X (Point{ iShift, iShift, 0 }) which is incorrect for Bx (primal X, dual
Y & Z); replace the P4 point so the stencil samples the (0, iShift, iShift)
corner (i.e. Point<int, dimension>{0, iShift, iShift}) while keeping the 0.25
weight so the four weights cover all Y/Z dual combinations and sum correctly.
| using Id = std::uint16_t; | ||
| using Idx = Id const; | ||
|
|
||
| template<typename NCells, template<typename, std::size_t> typename Indexes, typename Index> | ||
| static inline std::uint32_t idx(NCells const& nCells, Indexes<Index, dim> const& indexes) | ||
| { | ||
| if constexpr (dim == 1) | ||
| { | ||
| auto i = std::get<0>(params); | ||
| return idx(nCells, indexes[0]); | ||
|
|
||
| return data[i]; | ||
| } | ||
| else if constexpr (dim == 2) | ||
| return idx(nCells, indexes[0], indexes[1]); | ||
|
|
||
| if constexpr (dim == 2) | ||
| { | ||
| auto i = std::get<0>(params); | ||
| auto j = std::get<1>(params); | ||
| else if constexpr (dim == 3) | ||
| return idx(nCells, indexes[0], indexes[1], indexes[2]); | ||
| } | ||
|
|
||
| if constexpr (c_ordering) | ||
| return data[j + i * nCells[1]]; | ||
| else | ||
| return data[i + j * nCells[0]]; | ||
| } | ||
| static inline std::uint32_t idx(auto const /*nCells*/, Idx i) { return i; } | ||
|
|
||
| if constexpr (dim == 3) | ||
| { | ||
| auto i = std::get<0>(params); | ||
| auto j = std::get<1>(params); | ||
| auto k = std::get<2>(params); | ||
|
|
||
| if constexpr (c_ordering) | ||
| return data[k + j * nCells[2] + i * nCells[1] * nCells[2]]; | ||
| else | ||
| return data[i + j * nCells[0] + k * nCells[1] * nCells[0]]; | ||
| } | ||
| static inline std::uint32_t idx(auto const nCells, Idx i, Idx j) | ||
| { | ||
| if constexpr (c_ordering) | ||
| return j + i * nCells[1]; | ||
| else | ||
| return i + j * nCells[0]; | ||
| } | ||
| static inline std::uint32_t idx(auto const nCells, Idx i, Idx j, Idx k) | ||
| { | ||
| if constexpr (c_ordering) | ||
| return k + j * nCells[2] + i * nCells[1] * nCells[2]; | ||
| else | ||
| return i + j * nCells[0] + k * nCells[1] * nCells[0]; |
There was a problem hiding this comment.
Do not narrow indices to 16 bits
Line [22] changes the index alias to std::uint16_t, and the overloads at Lines [38]-[52] now accept that Idx. Any access past 65,535 silently wraps before computing the flat offset, so large grids (we already store shapes in std::uint32_t) will read/write the wrong cell. Please keep the index type at least 32-bit (or derive it from the shape entry type).
- using Id = std::uint16_t;
+ using Id = std::uint32_t;🤖 Prompt for AI Agents
In src/core/data/ndarray/ndarray_vector.hpp around lines 22 to 52, the index
alias was narrowed to std::uint16_t causing silent wrapping for dimensions
>65,535; change the index alias to a 32‑bit type (e.g. using Id = std::uint32_t)
or derive Id from the nCells/shape element type so Id and Idx are at least 32
bits, leaving the idx overloads as-is to accept the wider Idx and compute flat
offsets correctly.
| template<typename T> | ||
| NO_DISCARD inline T get_env_as(std::string const& key, T const& t) | ||
| { | ||
| if (auto e = get_env(key)) | ||
| return from_string<T>(*e); | ||
| return t; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for invalid environment variable values.
If the environment variable exists but contains an invalid value for type T, from_string will return a default-constructed T rather than the provided default parameter t. This creates confusing behavior where the caller cannot distinguish between "not set" and "invalid value" cases.
Consider one of these approaches:
- Catch conversion failures and return
t:
template<typename T>
NO_DISCARD inline T get_env_as(std::string const& key, T const& t)
{
if (auto e = get_env(key))
- return from_string<T>(*e);
+ {
+ std::stringstream ss(*e);
+ T result;
+ if (ss >> result && ss.eof())
+ return result;
+ // Invalid format: return default
+ }
return t;
}-
Return
std::optional<T>to signal parsing failures (requires updatingfrom_stringfirst per previous review) -
Throw an exception on invalid format with a clear error message indicating which environment variable had an invalid value
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
src/core/utilities/types.hpp lines 248-254: get_env_as currently returns a
default-constructed T when the env var exists but contains an invalid value;
change it to catch parsing failures and return the provided default instead.
Wrap the call to from_string<T>(*e) in a try/catch (catching the parsing
exception type or std::exception) and on any parse error return t; if
from_string does not yet throw on invalid input, update from_string to throw a
clear parsing exception for invalid formats so get_env_as can reliably catch and
return the caller-provided default.
| } | ||
|
|
||
|
|
||
| def permute(dic={}): |
There was a problem hiding this comment.
Fix mutable default argument.
Using a mutable default argument dic={} is a Python anti-pattern that can lead to subtle bugs, as the same dictionary instance is shared across all invocations.
Apply this diff:
-def permute(dic={}):
+def permute(dic=None):
+ if dic is None:
+ dic = {}
dic.update(simArgs.copy())🧰 Tools
🪛 Ruff (0.14.3)
121-121: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In tests/simulator/test_vtk_diagnostics.py around line 121, the function
signature uses a mutable default argument def permute(dic={}) which shares the
same dict across calls; change the signature to def permute(dic=None) and inside
the function set dic = {} if dic is None so each call gets a fresh dictionary,
preserving existing behavior but avoiding the mutable-default anti-pattern.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
src/core/data/ndarray/ndarray_vector.hpp (1)
22-23: Do not narrow indices to 16 bitsThis issue was flagged in a previous review but remains unaddressed. Line 22 defines
Idasstd::uint16_t, limiting indexing to 65,535 elements. Multi-dimensional grids stored withstd::uint32_tshapes (line 327) can easily exceed this limit (e.g., 300×300 = 90,000), causing silent wraparound and incorrect memory access.Apply this diff to use 32-bit indices:
- using Id = std::uint16_t; + using Id = std::uint32_t;src/diagnostic/detail/vtk_types/particle.hpp (1)
37-53: Particle diagnostics never write data.The
write_quantitylambda at Line 37-39 is empty, somodelView.visitHierarchydoes nothing and particle VTK-HDF5 output files remain empty. This breaks particle diagnostics when the "pharevtkhdf" format is selected.src/hdf5/detail/h5/h5_file.hpp (3)
105-114: Missing parent group creation will cause dataset creation to fail.Unlike
create_data_set(Line 100), this function doesn't callcreateGroupsToDataSet(path)before creating the dataset. If parent groups don't exist,h5file_.createDataSetwill fail.Apply this diff to add parent group creation:
template<typename Type> auto create_chunked_data_set(auto const& path, auto const chunk, auto const& dataspace) { if (exist(path)) return h5file_.getDataSet(path); + createGroupsToDataSet(path); HighFive::DataSetCreateProps props; props.add(HighFive::Chunking{chunk}); return h5file_.createDataSet(path, dataspace, HighFive::create_datatype<Type>(), props); }
249-253: Replace assertion with exception for production safety.The
assert(exist(s))at Line 251 is compiled out in release builds (whenNDEBUGis defined), potentially causing undefined behavior if a non-existent dataset is accessed.Apply this diff:
auto getDataSet(std::string const& s) { - assert(exist(s)); + if (!exist(s)) + throw std::runtime_error("Dataset does not exist: " + s); return h5file_.getDataSet(s); }
255-258: Fix incorrect move constructor and assignment signatures.Lines 256 and 258 declare move operations with
const&&, which is incorrect. Move constructors and move assignment operators should take non-const rvalue references (&&) notconst&&.Apply this diff:
HighFiveFile(HighFiveFile const&) = delete; -HighFiveFile(HighFiveFile const&&) = delete; +HighFiveFile(HighFiveFile&&) = delete; HighFiveFile& operator=(HighFiveFile const&) = delete; -HighFiveFile& operator=(HighFiveFile const&&) = delete; +HighFiveFile& operator=(HighFiveFile&&) = delete;tests/simulator/test_vtk_diagnostics.py (1)
120-120: Fix mutable default argument anti-pattern.Using
dic={}is a Python anti-pattern where the same dictionary instance is shared across all invocations, leading to subtle bugs.Apply this diff:
-def permute(dic={}): +def permute(dic=None): + if dic is None: + dic = {} dic.update(simArgs.copy())Based on learnings
src/diagnostic/detail/vtkh5_writer.hpp (1)
177-183: File flags not reset after write causes data loss.The
file_flagsentry retainsHiFile::Truncatefrom the initialthis->flagsassignment, causing subsequent dumps to reopen files in truncate mode and erase earlier time steps. After writing completes, reset each diagnostic's flag toREAD_WRITEto preserve data across dumps.Apply this diff:
for (auto* diagnostic : diagnostics) typeWriters_.at(diagnostic->type)->write(*diagnostic); + + for (auto* diagnostic : diagnostics) + file_flags[diagnostic->type + diagnostic->quantity] = READ_WRITE;src/core/data/grid/gridlayout.hpp (1)
1203-1209: Unsigned wraparound in shrink operation.The expression
-1 * nbrGhosts(centerings[i])on line 1207 performs unsigned arithmetic becausenbrGhostsreturnsstd::uint32_t, causing wraparound instead of shrinking the box.src/core/data/grid/gridlayoutimplyee.hpp (1)
529-536: Fix P4 offset in 3D Bx→moment interpolation.For Bx (primal in X, dual in Y & Z), the 3D averaging must cover all Y/Z dual combinations while keeping X at 0. P4 at line 534 incorrectly shifts X (
{iShift, iShift, 0}), so the stencil never samples the(0, iShift, iShift)corner.Apply this diff:
- constexpr WeightPoint<dimension> P4{Point<int, dimension>{iShift, iShift, 0}, 0.25}; + constexpr WeightPoint<dimension> P4{Point<int, dimension>{0, iShift, iShift}, 0.25};
🧹 Nitpick comments (4)
src/core/data/ndarray/ndarray_vector.hpp (1)
38-53: Index arithmetic is correct, but clarify parameter typesThe index calculation formulas for both C and Fortran ordering are mathematically correct. However, the
auto constfor thenCellsparameter is less explicit than a concrete type.Consider making the
nCellsparameter type explicit for clarity:- static inline std::uint32_t idx(auto const /*nCells*/, Idx i) { return i; } + static inline std::uint32_t idx(std::array<std::uint32_t, dim> const& /*nCells*/, Idx i) { return i; } - static inline std::uint32_t idx(auto const nCells, Idx i, Idx j) + static inline std::uint32_t idx(std::array<std::uint32_t, dim> const& nCells, Idx i, Idx j) { // ... } - static inline std::uint32_t idx(auto const nCells, Idx i, Idx j, Idx k) + static inline std::uint32_t idx(std::array<std::uint32_t, dim> const& nCells, Idx i, Idx j, Idx k) { // ... }src/diagnostic/detail/vtk_types/meta.hpp (1)
11-31: Clarify the purpose of this stub implementation.The
MetaDiagnosticWriterhas emptywrite()andcompute()methods, unlike the h5::MetaDiagnosticWriter which handles tags data. If this is a placeholder for future VTK-specific meta-diagnostics, consider adding a comment explaining the intended functionality or temporary nature.Verify whether this stub is intentional or if meta-diagnostic writing should be implemented for VTK format.
src/diagnostic/detail/vtkh5_writer.hpp (1)
130-130: Remove hard-coded maxLevel or document rationale.The TODO indicates
maxLevel = 10needs parametrization. Either accept it through the constructor/configuration or document why 10 is a safe default.src/diagnostic/diagnostic_model_view.hpp (1)
1-2: Consider updating header guard to match project convention.The header guard could follow the full-path pattern:
PHARE_DIAGNOSTIC_DIAGNOSTIC_MODEL_VIEW_HPPinstead ofDIAGNOSTIC_MODEL_VIEW_HPP.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
pyphare/pyphare/pharein/__init__.py(1 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)pyphare/pyphare/pharesee/__init__.py(1 hunks)src/amr/physical_models/hybrid_model.hpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(3 hunks)src/core/data/grid/gridlayoutdefs.hpp(1 hunks)src/core/data/grid/gridlayoutimplyee.hpp(27 hunks)src/core/data/ndarray/ndarray_vector.hpp(5 hunks)src/core/data/tensorfield/tensorfield.hpp(1 hunks)src/core/utilities/algorithm.hpp(2 hunks)src/core/utilities/box/box.hpp(2 hunks)src/core/utilities/types.hpp(3 hunks)src/diagnostic/detail/h5writer.hpp(1 hunks)src/diagnostic/detail/vtk_types/electromag.hpp(1 hunks)src/diagnostic/detail/vtk_types/fluid.hpp(1 hunks)src/diagnostic/detail/vtk_types/info.hpp(1 hunks)src/diagnostic/detail/vtk_types/meta.hpp(1 hunks)src/diagnostic/detail/vtk_types/particle.hpp(1 hunks)src/diagnostic/detail/vtkh5_type_writer.hpp(1 hunks)src/diagnostic/detail/vtkh5_writer.hpp(1 hunks)src/diagnostic/diagnostic_manager.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(7 hunks)src/diagnostic/diagnostics.hpp(2 hunks)src/hdf5/detail/h5/h5_file.hpp(2 hunks)src/phare/CMakeLists.txt(0 hunks)src/phare/phare_init.py(0 hunks)src/phare/phare_init_small.py(0 hunks)tests/simulator/CMakeLists.txt(2 hunks)tests/simulator/__init__.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/test_vtk_diagnostics.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/phare/phare_init.py
- src/phare/CMakeLists.txt
- src/phare/phare_init_small.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/simulator/init.py
- src/diagnostic/diagnostic_manager.hpp
- pyphare/pyphare/pharesee/init.py
- src/core/utilities/types.hpp
- pyphare/pyphare/pharein/init.py
- src/core/data/grid/gridlayoutdefs.hpp
- src/core/data/tensorfield/tensorfield.hpp
- tests/simulator/CMakeLists.txt
- src/amr/resources_manager/amr_utils.hpp
- src/diagnostic/diagnostics.hpp
- tests/simulator/refinement/test_2d_10_core.py
- src/core/utilities/algorithm.hpp
- src/diagnostic/detail/vtkh5_type_writer.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/diagnostic/detail/h5writer.hppsrc/diagnostic/detail/vtk_types/fluid.hppsrc/core/data/grid/gridlayoutimplyee.hppsrc/core/utilities/box/box.hppsrc/hdf5/detail/h5/h5_file.hppsrc/diagnostic/detail/vtk_types/particle.hppsrc/amr/physical_models/hybrid_model.hppsrc/diagnostic/diagnostic_model_view.hppsrc/core/data/grid/gridlayout.hppsrc/diagnostic/detail/vtk_types/electromag.hppsrc/diagnostic/detail/vtkh5_writer.hppsrc/diagnostic/detail/vtk_types/meta.hppsrc/core/data/ndarray/ndarray_vector.hppsrc/diagnostic/detail/vtk_types/info.hpp
🧠 Learnings (6)
📓 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-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/amr/CMakeLists.txt:103-105
Timestamp: 2025-07-09T17:18:05.771Z
Learning: In the PHARE project, `HighFive` is a CMake option defined in `res/cmake/options.cmake` as `option(HighFive "Build with highfive usage" ON)`, not a target that would be created by find_package(). The condition `if (HighFive)` correctly checks this option.
Applied to files:
src/diagnostic/detail/h5writer.hpp
📚 Learning: 2024-10-09T08:32:15.667Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 859
File: pyphare/pyphare/pharesee/hierarchy/hierarchy.py:26-26
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Mutable default arguments can lead to unexpected behavior because the default value is shared between all calls to the function. Replacing the mutable default with `None` and initializing it within the function ensures that a new list is created each time the function is called, avoiding this issue.
Applied to files:
tests/simulator/test_vtk_diagnostics.py
📚 Learning: 2024-07-23T09:51:46.761Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 859
File: pyphare/pyphare/pharesee/hierarchy/fromh5.py:27-34
Timestamp: 2024-07-23T09:51:46.761Z
Learning: Mutable default arguments like lists or dictionaries can lead to unexpected behavior because the same instance of the mutable object is shared across all calls to the function. It is a good practice to use `None` as the default value and initialize the mutable object inside the function.
Applied to files:
tests/simulator/test_vtk_diagnostics.py
📚 Learning: 2024-10-09T08:32:15.667Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/core/data/grid/gridlayout.hpp:805-821
Timestamp: 2024-10-09T08:32:15.667Z
Learning: In the `AMRToLocal` function, before converting to `uint32_t`, assert that `local >= 0` to ensure values are valid.
Applied to files:
src/core/data/grid/gridlayout.hpp
📚 Learning: 2024-09-09T13:57:02.285Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Applied to files:
src/diagnostic/detail/vtkh5_writer.hpp
🧬 Code graph analysis (11)
src/diagnostic/detail/vtk_types/fluid.hpp (3)
src/diagnostic/detail/vtkh5_type_writer.hpp (8)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)boxes(57-71)boxes(57-57)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)
src/core/utilities/box/box.hpp (2)
src/core/utilities/types.hpp (2)
product(265-268)product(265-265)pyphare/pyphare/core/box.py (1)
shape(32-34)
src/diagnostic/detail/vtk_types/particle.hpp (3)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)
tests/simulator/test_vtk_diagnostics.py (7)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(6-9)pyphare/pyphare/simulator/simulator.py (2)
startMPI(41-45)reset(258-269)tests/simulator/__init__.py (4)
SimulatorTest(201-267)tearDown(204-205)unique_diag_dir_for_test_case(250-254)run(246-248)tests/diagnostic/__init__.py (1)
dump_all_diags(15-62)pyphare/pyphare/pharein/simulation.py (1)
simulation_domain(967-968)pyphare/pyphare/pharein/maxwellian_fluid_model.py (1)
MaxwellianFluidModel(8-303)pyphare/pyphare/pharein/electron_model.py (1)
ElectronModel(22-35)
src/diagnostic/diagnostic_model_view.hpp (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (10)
patch(146-154)patch(146-146)ions(407-416)ions(407-407)ions(420-445)ions(420-420)ions(447-480)ions(447-447)ions(492-531)ions(492-493)src/amr/resources_manager/amr_utils.hpp (2)
visitHierarchy(244-253)visitHierarchy(244-245)src/core/data/ions/ions.hpp (1)
forward_as_tuple(225-229)src/core/data/ions/ion_population/ion_population.hpp (1)
forward_as_tuple(109-113)
src/core/data/grid/gridlayout.hpp (2)
src/core/hybrid/hybrid_quantities.hpp (4)
auto(45-45)auto(46-46)auto(47-47)auto(48-48)src/amr/data/field/coarsening/field_coarsen_index_weight.hpp (3)
centering(27-102)centering(30-48)centering(31-32)
src/diagnostic/detail/vtk_types/electromag.hpp (3)
src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (8)
diagnostic(87-92)diagnostic(87-87)lvl(267-272)lvl(267-267)ilvl(388-417)ilvl(388-388)boxes(57-71)boxes(57-57)src/diagnostic/detail/types/electromag.hpp (1)
PHARE(8-173)
src/diagnostic/detail/vtkh5_writer.hpp (3)
src/diagnostic/diagnostic_model_view.hpp (1)
dimension(30-60)src/diagnostic/detail/h5writer.hpp (5)
level(72-73)fileStr(81-87)fileStr(81-81)diagnostic(94-98)diagnostic(94-94)src/diagnostic/detail/vtkh5_type_writer.hpp (6)
level(298-334)level(298-298)level(336-340)level(336-336)diagnostic(87-92)diagnostic(87-87)
src/diagnostic/detail/vtk_types/meta.hpp (5)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtkh5_type_writer.hpp (2)
diagnostic(87-92)diagnostic(87-87)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)src/diagnostic/detail/types/meta.hpp (1)
PHARE(7-143)
src/core/data/ndarray/ndarray_vector.hpp (1)
src/core/utilities/types.hpp (2)
product(265-268)product(265-265)
src/diagnostic/detail/vtk_types/info.hpp (7)
src/diagnostic/detail/vtk_types/electromag.hpp (2)
write(30-55)write(30-30)src/diagnostic/detail/vtk_types/fluid.hpp (4)
write(38-71)write(38-38)diagnostic(29-33)diagnostic(29-30)src/diagnostic/detail/vtk_types/meta.hpp (2)
write(29-31)write(29-29)src/diagnostic/detail/vtk_types/particle.hpp (2)
write(31-54)write(31-31)src/diagnostic/detail/h5writer.hpp (2)
diagnostic(94-98)diagnostic(94-94)src/diagnostic/detail/vtkh5_type_writer.hpp (2)
diagnostic(87-92)diagnostic(87-87)src/diagnostic/detail/vtkh5_writer.hpp (2)
diagnostic(97-101)diagnostic(97-97)
🪛 Ruff (0.14.3)
tests/simulator/test_vtk_diagnostics.py
1-1: Shebang is present but file is not executable
(EXE001)
26-26: Unused function argument: xyz
(ARG001)
33-33: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
40-40: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
43-43: Unused function argument: xyz
(ARG001)
46-46: Unused function argument: xyz
(ARG001)
53-53: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
60-60: Do not assign a lambda expression, use a def
Rewrite _ as a def
(E731)
63-63: Unused function argument: xyz
(ARG001)
66-66: Unused function argument: xyz
(ARG001)
69-69: Unused function argument: xyz
(ARG001)
120-120: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (25)
src/core/utilities/box/box.hpp (3)
97-97: LGTM!The per-dimension shape accessor is clean and consistent with the existing
shape()method. The implementation correctly computes the extent for dimensioni.
99-99: Excellent type safety improvement!Explicitly passing
std::size_t{1}ensures theproductfunction returnsstd::size_tinstead ofint, preventing potential overflow for large boxes and providing the semantically correct return type for size calculations.
194-197: LGTM!The inequality operator is correctly implemented, comparing both the box pointer and the current index. This is a standard iterator requirement and the implementation is clean and correct.
src/core/data/ndarray/ndarray_vector.hpp (3)
25-36: LGTM: Clean dimension dispatchThe dispatcher correctly forwards to dimension-specific overloads using compile-time branching.
55-70: LGTM: Proper bounds checking in accessorsThe
at()functions correctly compute the flat index and verify it against the total array size before access.
101-101: LGTM: Consistent refactoring across all usage sitesAll call sites have been properly updated to use the simplified
NdArrayViewerinterface without theDataTypetemplate parameter.Also applies to: 138-138, 161-161, 167-167, 291-291, 303-303
src/amr/physical_models/hybrid_model.hpp (1)
31-39: Changes are valid for C++20 standard and safe to merge.The removal of
typenamefrom dependent type aliases in lines 31, 32, 35, 36, and 39 is valid C++20 syntax. The project's root CMakeLists.txt enforces C++20 globally, andhybrid_model.hppis only included within C++20 contexts (src/amr/tree, tests, and main solver). The changes align with the project's preference for clean, minimal code and pose no compilation risk.pyphare/pyphare/pharein/simulation.py (1)
484-484: LGTM! Format string addition is consistent.The addition of "pharevtkhdf" follows the existing naming pattern and properly extends diagnostic format support.
src/diagnostic/detail/h5writer.hpp (1)
6-17: Header restructuring aligns with VTK-HDF5 writer infrastructure.The include reordering and addition of diagnostic type headers support the new writer pathway introduced in this PR. The removal of forward declarations in favor of full includes is appropriate given the template instantiation requirements.
src/diagnostic/detail/vtk_types/particle.hpp (1)
7-7: Resolve or remove the uncertainty comment.Line 7's comment "not sure this makes sense" suggests unresolved design concerns. Please clarify whether this writer should be included in the release or deferred until the implementation is complete.
src/diagnostic/detail/vtk_types/electromag.hpp (1)
29-55: Electromagnetic diagnostic writer implementation is complete and correct.The implementation properly:
- Iterates over levels via
modelView.onLevels- Initializes file structure with
initFileLevelandinitTensorFieldFileLevel- Writes tensor fields via the write_quantity lambda
- Follows the established pattern from other diagnostic writers
src/diagnostic/detail/vtk_types/fluid.hpp (1)
37-71: Fluid diagnostic writer is correctly implemented.The implementation properly:
- Uses
isActiveDiaghelper to reduce duplication when checking diagnostic quantities- Conditionally writes charge_density, mass_density, and bulkVelocity fields
- Distinguishes between scalar fields (
writeField) and vector fields (writeTensorField<1>)- Initializes file structure per-level before writing
src/diagnostic/detail/vtk_types/info.hpp (1)
30-33: Verify that empty write implementation is intentional.The
writemethod has an empty body. If info diagnostics don't require VTK-HDF5 output, this is acceptable. However, please confirm whether this writer is complete or deferred for future work.tests/simulator/test_vtk_diagnostics.py (1)
150-174: Test correctly validates both diagnostic formats.The test properly:
- Creates simulations with both "phareh5" and "pharevtkhdf" formats
- Verifies simulation setup (cell count)
- Dumps diagnostics and runs simulations
- Resets state between runs to ensure clean test isolation
src/core/data/grid/gridlayout.hpp (1)
1034-1053: LGTM!The new
BxToMoments,ByToMoments, andBzToMomentsaccessors follow the existing pattern for moment projection helpers and correctly delegate to the implementation layer.src/core/data/grid/gridlayoutimplyee.hpp (3)
427-427: LGTM!The refactoring to use CTAD (
std::array{...}) instead of explicit template parameters improves readability while maintaining type safety.Also applies to: 433-433, 439-439, 460-460, 466-466, 472-472, 493-493, 498-498, 505-506, 621-621, 627-627, 633-633, 651-651, 657-657, 663-663, 681-681, 686-686, 692-692, 710-710, 716-716, 722-722, 739-739, 745-745, 751-751, 769-769, 774-774, 780-780, 796-796, 805-805, 822-822, 838-838, 843-843, 849-849, 867-867, 873-873, 880-880, 897-897, 907-907, 923-923, 942-942, 948-948, 954-954, 972-972, 978-978, 985-985, 1003-1003, 1008-1008, 1015-1015, 1032-1032, 1041-1041, 1057-1057, 1074-1074, 1080-1080, 1086-1086, 1102-1102, 1107-1107, 1112-1112, 1128-1128, 1133-1133, 1138-1138, 1154-1154, 1159-1159, 1164-1164
541-569: LGTM!
ByToMomentscorrectly implements the Dpd→Ppp projection by averaging over all X/Z dual node combinations while keeping Y primal. The 3D stencil properly covers all four required points with equal weights.
574-604: LGTM!
BzToMomentscorrectly implements the Ddp→Ppp projection by averaging over all X/Y dual node combinations while keeping Z primal. The 3D stencil properly covers all four required points with equal weights.src/diagnostic/diagnostic_model_view.hpp (7)
37-37: LGTM! Clear type alias addition.The new
Fieldalias follows the established pattern and improves code readability.
70-80: LGTM! Consistent refactoring to temporary storage.The switch from
sumTensor_totmpTensor_follows the established pattern in the codebase (similar to patterns inhybrid_hybrid_messenger_strategy.hpp).
145-154: LGTM! Helper provides convenient access to level boxes.The method provides a clear interface for collecting patch boxes at a given level, consistent with the existing visitation patterns.
156-167: LGTM! Well-designed accessor methods with compile-time safety.The template-based
tmpTensorField()with static assertion provides type-safe access to temporary storage, and the rank-based dispatch is clean and efficient.
169-172: LGTM! Follows established resource view pattern.The method mirrors the pattern used in
IonsandIonPopulationclasses, providing a standard interface for accessing compile-time resources.
183-183: LGTM! Consistent with temporary storage refactoring.The update to use
tmpTensor_.name()aligns with the broader refactoring fromsumTensor_totmpTensor_.
194-194: LGTM! Correct template parameter usage.The use of
GridLayoutis consistent with the type alias defined at line 33.
| NO_DISCARD constexpr static auto centering(auto const& hasQuantity) | ||
| { | ||
| return centering(hasQuantity.physicalQuantity()); | ||
| } |
There was a problem hiding this comment.
Remove ambiguous overload.
This unconstrained centering(auto const& hasQuantity) overload conflicts with the constrained template overload at lines 894-899. For any type with a physicalQuantity() method, both overloads are viable, and the compiler will reject the call as ambiguous since neither is more specialized than the other.
Remove this overload; the constrained version at lines 894-899 already handles all types with physicalQuantity():
- NO_DISCARD constexpr static auto centering(auto const& hasQuantity)
- {
- return centering(hasQuantity.physicalQuantity());
- }
-📝 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.
| NO_DISCARD constexpr static auto centering(auto const& hasQuantity) | |
| { | |
| return centering(hasQuantity.physicalQuantity()); | |
| } |
🤖 Prompt for AI Agents
In src/core/data/grid/gridlayout.hpp around lines 882-885, remove the
unconstrained NO_DISCARD constexpr static auto centering(auto const&
hasQuantity) overload because it conflicts with the constrained template
overload at lines 894-899; delete the entire ambiguous overload so calls will
resolve to the constrained version that already handles types exposing
physicalQuantity(), then rebuild to ensure no remaining ambiguities.
|
|
||
| } // namespace PHARE::diagnostic::vtkh5 | ||
|
|
||
| #endif /* PHARE_DIAGNOSTIC_DETAIL_TYPES_META_H */ |
There was a problem hiding this comment.
Header guard comment mismatch.
The closing comment references PHARE_DIAGNOSTIC_DETAIL_TYPES_META_H but the opening guard is PHARE_DIAGNOSTIC_DETAIL_VTK_TYPES_META_HPP.
Apply this diff:
-#endif /* PHARE_DIAGNOSTIC_DETAIL_TYPES_META_H */
+#endif /* PHARE_DIAGNOSTIC_DETAIL_VTK_TYPES_META_HPP */📝 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.
| #endif /* PHARE_DIAGNOSTIC_DETAIL_TYPES_META_H */ | |
| #endif /* PHARE_DIAGNOSTIC_DETAIL_VTK_TYPES_META_HPP */ |
🤖 Prompt for AI Agents
In src/diagnostic/detail/vtk_types/meta.hpp around line 37, the closing header
guard comment incorrectly references PHARE_DIAGNOSTIC_DETAIL_TYPES_META_H;
update that comment to match the opening guard
PHARE_DIAGNOSTIC_DETAIL_VTK_TYPES_META_HPP so the closing line reads the same
identifier as the opening guard.
| H5Writer& operator&(H5Writer const&) = delete; | ||
| H5Writer& operator&(H5Writer&&) = delete; |
There was a problem hiding this comment.
Incorrect operator deletion - should be assignment, not address-of.
Lines 62-63 delete operator& (address-of operator), which is highly unusual. You likely intended to delete the copy and move assignment operators instead.
Apply this diff:
- H5Writer& operator&(H5Writer const&) = delete;
- H5Writer& operator&(H5Writer&&) = delete;
+ H5Writer& operator=(H5Writer const&) = delete;
+ H5Writer& operator=(H5Writer&&) = delete;📝 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.
| H5Writer& operator&(H5Writer const&) = delete; | |
| H5Writer& operator&(H5Writer&&) = delete; | |
| H5Writer& operator=(H5Writer const&) = delete; | |
| H5Writer& operator=(H5Writer&&) = delete; |
🤖 Prompt for AI Agents
In src/diagnostic/detail/vtkh5_writer.hpp around lines 62-63, the code
mistakenly deletes the address-of operator (operator&) instead of the copy and
move assignment operators; change those two declarations to delete the copy
assignment operator (H5Writer& operator=(H5Writer const&) = delete;) and the
move assignment operator (H5Writer& operator=(H5Writer&&) = delete;) so the
class properly prevents assignment while keeping the address-of operator intact.
|
|
||
| } // namespace PHARE::diagnostic::vtkh5 | ||
|
|
||
| #endif /* PHARE_DETAIL_DIAGNOSTIC_HIGHFIVE_H */ |
There was a problem hiding this comment.
Header guard comment mismatch.
The closing comment references PHARE_DETAIL_DIAGNOSTIC_HIGHFIVE_H but the opening guard is PHARE_DIAGNOSTIC_DETAIL_VTK_H5_WRITER_HPP.
Apply this diff:
-#endif /* PHARE_DETAIL_DIAGNOSTIC_HIGHFIVE_H */
+#endif /* PHARE_DIAGNOSTIC_DETAIL_VTK_H5_WRITER_HPP */📝 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.
| #endif /* PHARE_DETAIL_DIAGNOSTIC_HIGHFIVE_H */ | |
| #endif /* PHARE_DIAGNOSTIC_DETAIL_VTK_H5_WRITER_HPP */ |
🤖 Prompt for AI Agents
In src/diagnostic/detail/vtkh5_writer.hpp around line 206, the header guard
closing comment is incorrect: it references PHARE_DETAIL_DIAGNOSTIC_HIGHFIVE_H
while the opening guard uses PHARE_DIAGNOSTIC_DETAIL_VTK_H5_WRITER_HPP; update
the closing comment to match the opening guard identifier exactly (change the
trailing comment after the #endif to /*
PHARE_DIAGNOSTIC_DETAIL_VTK_H5_WRITER_HPP */) so the guard names are consistent.
| amr::visitHierarchy<GridLayout>(hierarchy_, *model_.resourcesManager, | ||
| std::forward<Action>(action), minLevel, maxLevel, *this, | ||
| model_); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that all action callsites handle the updated signature.
The visitHierarchy method now forwards both *this and model_ to actions, changing the action signature. Ensure all call sites are updated to accept both parameters.
🏁 Script executed:
#!/bin/bash
# Description: Find all call sites of visitHierarchy to verify they handle the new signature
# Search for visitHierarchy calls on ModelView or related types
rg -nP --type=cpp -C5 'visitHierarchy\s*\(' -g '!src/diagnostic/diagnostic_model_view.hpp'Length of output: 8406
I need to verify that all action lambdas accept the new variadic parameters. Let me search for the actual lambda definitions:Now let me check the specific lambda definitions to verify they handle the new parameters:
🏁 Script executed:
#!/bin/bash
# Search for collectPatchAttributes, writePatch lambda definitions to verify their signatures
echo "=== Checking collectPatchAttributes lambda ==="
rg -nA10 'auto const collectPatchAttributes' src/diagnostic/detail/h5writer.hpp
echo ""
echo "=== Checking writePatch lambda ==="
rg -nA5 'auto const writePatch' src/diagnostic/detail/h5writer.hpp
echo ""
echo "=== Checking write_quantity and gatherParticleCounts lambdas ==="
rg -nB2 'write_quantity|gatherParticleCounts' src/diagnostic/detail/vtk_types/particle.hpp src/diagnostic/detail/types/info.hppLength of output: 460
🏁 Script executed:
#!/bin/bash
# Search for collectPatchAttributes, writePatch lambda definitions to verify their signatures
echo "=== Checking collectPatchAttributes lambda ==="
rg -nA10 'auto const collectPatchAttributes' src/diagnostic/detail/
echo ""
echo "=== Checking writePatch lambda ==="
rg -nA5 'auto const writePatch' src/diagnostic/detail/
echo ""
echo "=== Checking write_quantity lambda ==="
rg -nB3 -A3 'write_quantity\s*=' src/diagnostic/detail/vtk_types/
echo ""
echo "=== Checking gatherParticleCounts lambda ==="
rg -nB3 -A3 'gatherParticleCounts\s*=' src/diagnostic/detail/types/Length of output: 1997
Update all visitHierarchy call sites and action signatures to match the new variadic parameter passing.
The new visitHierarchy signature forwards both *this (ModelView) and model_ as variadic arguments to actions. All 9 call sites currently use the old 3-argument style and will fail to compile. Additionally, action lambdas must be updated to accept these new parameters—either by using auto&&... pattern or explicit parameter declarations.
Call sites requiring updates:
src/diagnostic/detail/h5writer.hpp:295(collectPatchAttributes action)src/diagnostic/detail/h5writer.hpp:330(writePatch action)src/diagnostic/detail/vtk_types/particle.hpp:51(write_quantity action)src/diagnostic/detail/types/fluid.hpp:108, 112, 126(interpolate and momentum tensor actions)src/diagnostic/detail/types/info.hpp:112(gatherParticleCounts action)src/diagnostic/detail/vtk_types/electromag.hpp:52(write_quantity action)src/diagnostic/detail/vtk_types/fluid.hpp:68(write_quantity action)
🤖 Prompt for AI Agents
In src/diagnostic/diagnostic_model_view.hpp around lines 96 to 98, update all
visitHierarchy call sites listed in the review (the 9 files/locations) to match
the new variadic parameter API by passing the extra context arguments (*this and
model_) through visitHierarchy where required, and update each action
lambda/signature to accept the additional forwarded parameters (use either a
trailing auto&&... ctx parameter or a generic auto&&... param pack like
auto&&... ctx or explicit parameters for ModelView& and Model&). Ensure lambdas
forward or ignore the extra args correctly (e.g., accept auto&&... and not use
them if unnecessary), keep forwarding of the Action
(std::forward<Action>(action)), and rebuild to fix compilation errors.
| Field tmpField_{"PHARE_sumField", core::HybridQuantity::Scalar::rho}; | ||
| VecField tmpVec_{"PHARE_sumVec", core::HybridQuantity::Vector::V}; | ||
| TensorFieldT tmpTensor_{"PHARE_sumTensor", core::HybridQuantity::Tensor::M}; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that temporary storage is properly allocated before use.
The temporary fields are initialized with names and quantities, but actual memory allocation should occur through the resources manager. Confirm that tmpField_ and tmpVec_ are allocated when needed, similar to how tmpTensor_ is used in fillPopMomTensor.
🏁 Script executed:
#!/bin/bash
# Description: Search for allocation calls for tmpField_ and tmpVec_ to verify proper initialization
# Search for allocate calls with tmpField_ or tmpVec_
rg -nP --type=cpp -C3 'allocate\s*\(\s*(tmpField_|tmpVec_|tmpTensor_)'
# Also search for enumerate usage with these fields
rg -nP --type=cpp -C3 'enumerate\s*\([^)]*\b(tmpField_|tmpVec_|tmpTensor_)\b'Length of output: 1387
I need to search more broadly to understand if tmpField_ and tmpVec_ are actually used anywhere in the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all usages of tmpField_ and tmpVec_ to see where they're used
rg -nP --type=cpp '\b(tmpField_|tmpVec_)\b' src/diagnostic/diagnostic_model_view.hpp
# Also search in the entire diagnostic directory for any references
rg -nP --type=cpp '\b(tmpField_|tmpVec_)\b' src/diagnostic/
Length of output: 953
I need to understand how the tuple at line 171 is used and whether Field/VecField types auto-allocate memory.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where the tuple from line 171 might be consumed - look for MTAlgos usage
rg -nP --type=cpp 'MTAlgos\[' src/diagnostic/ -A2 -B2
# Also search for Field and VecField class definitions to understand allocation patterns
rg -nP --type=hpp '\bclass\s+(Field|VecField)\b' --max-count=5
Length of output: 497
Let me search for the Field and VecField definitions using the correct file type:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Field and VecField class definitions using cpp type
rg -nP --type=cpp 'class\s+(Field|VecField)\b' -B2 -A5
# Also check what parameters the constructors take
rg -nP --type=cpp 'Field\s*\{.*"PHARE_' -B2 -A2
Length of output: 490
Let me examine the Field constructor and understand the allocation model:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Field constructor implementation
rg -nP --type=cpp 'Field\s*\(' src/core/data/field/field.hpp -A10 | head -50
# Also search for how resources manager enumerate works with allocation
rg -nP --type=cpp 'enumerate\s*\(' src/diagnostic/ -B2 -A5
Length of output: 2013
Let me verify if the accessor methods are actually called elsewhere:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for calls to tmpField() and tmpVecField()
rg -nP --type=cpp 'tmpField\(\)|tmpVecField\(\)'
# Also search for broader patterns that might consume the diagnostic data
rg -nP --type=cpp 'MTAlgos.*fillData|\.fillData\(' src/diagnostic/ -B3 -A3
Length of output: 1000
Found it! Let me examine how tmpField() is used in the vtkh5_type_writer:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get context around line 157 in vtkh5_type_writer.hpp
sed -n '150,180p' src/diagnostic/detail/vtkh5_type_writer.hpp
# Also search for what methods are called on the tmp field
rg -nP --type=cpp 'tmpField\(\)' -A10 src/diagnostic/detail/vtkh5_type_writer.hpp
Length of output: 1451
Let me check what convert_to_fortran_primal does with the field:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for convert_to_fortran_primal implementation
rg -nP --type=cpp 'convert_to_fortran_primal\s*\(' -A15 | head -60
# Also check if there's any allocation of tmpField_ elsewhere in the model view
rg -nP --type=cpp 'tmpField_\s*=' src/diagnostic/ -B2 -A2
Length of output: 4130
Let me verify if there's any late initialization of these fields or if the rm.enumerate pattern should be applied:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any setBuffer or assignment that might allocate tmpField_/tmpVec_
rg -nP --type=cpp 'setBuffer|\.data\s*=' src/diagnostic/diagnostic_model_view.hpp -B3 -A3
# Check if rm.enumerate is ever used with tmpField_ or tmpVec_ anywhere
rg -nP --type=cpp 'enumerate.*tmp(Field|Vec)'
# Also search for initialization patterns in constructors of the model view
rg -nP --type=cpp 'DiagnosticModelView\s*\(' src/diagnostic/ -A30 | head -80
Length of output: 0
Allocate tmpField_ and tmpVec_ using rm.enumerate() before use.
Evidence confirms these fields are used unallocated. While tmpTensor_ is properly allocated via rm.enumerate() at lines 70 and 77, tmpField_ and tmpVec_ retain null pointers from their default constructors. This causes undefined behavior when vtkh5_type_writer.hpp:157-158 retrieves tmpField() and passes it to convert_to_fortran_primal(), which attempts to access .data() (see algorithm.hpp:109).
Apply the same allocation pattern used for tmpTensor_: wrap tmpField_ and tmpVec_ usage with rm.enumerate() calls to allocate them within patch loops.
🤖 Prompt for AI Agents
In src/diagnostic/diagnostic_model_view.hpp around lines 219-221, tmpField_ and
tmpVec_ are left unallocated and used later causing UB; allocate them using
rm.enumerate() in the same places/pattern as tmpTensor_ (see the rm.enumerate()
calls around lines ~70 and ~77) so that each patch loop obtains initialized
storage before accessing tmpField() and tmpVec(); wrap their
usage/initialization with rm.enumerate() calls inside the patch loops to mirror
tmpTensor_ allocation.
2 participants
Write diagnostics in VTKHDF file format
tested via manual inspection in paraview
We should probably use the vtk module in a test and generate some artifacts