Conversation
📝 WalkthroughWalkthroughThis update introduces a comprehensive debugging and diagnostics framework across core AMR, solver, and simulation components. It adds debug macros, scoped instrumentation, runtime consistency checks, and utility functions for field and box manipulation. Several function signatures are updated for const-correctness and efficiency, and new public utility and accessor methods are provided for resource and grid management. Changes
Sequence Diagram(s)sequenceDiagram
participant Simulator
participant MultiPhysicsIntegrator
participant HybridLevelInitializer
participant HybridHybridMessengerStrategy
participant SolverPPC
participant Debuggerino
Simulator->>Debuggerino: PHARE_DEBUG_SCOPE("Simulator/initialize/")
Simulator->>MultiPhysicsIntegrator: initializeLevelIntegrator()
MultiPhysicsIntegrator->>Debuggerino: PHARE_DEBUG_SET(...), PHARE_DEBUG_SCOPE(...)
MultiPhysicsIntegrator->>HybridLevelInitializer: initialize()
HybridLevelInitializer->>Debuggerino: PHARE_DEBUG_SET(...), PHARE_DEBUG_SCOPE(...)
HybridLevelInitializer->>HybridHybridMessengerStrategy: prepareStep()
HybridLevelInitializer->>Debuggerino: PHARE_DEBUG_CHECK_LEVEL(...)
MultiPhysicsIntegrator->>HybridHybridMessengerStrategy: synchronize()
HybridHybridMessengerStrategy->>Debuggerino: PHARE_DEBUG_SCOPE(...), PHARE_DEBUG_CHECK_LEVEL(...)
Simulator->>SolverPPC: advanceLevel()
SolverPPC->>Debuggerino: PHARE_DEBUG_SCOPE(...), PHARE_DEBUG_CHECK_ALL_OVERLAPS(...)
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
4-41: Consider organizing includes more systematically.While the new includes are necessary for the debug functionality, the organization could be improved by grouping related headers together (e.g., all SAMRAI headers, all core headers, etc.) rather than having them interspersed.
+// Core headers +#include "core/def/phare_mpi.hpp" // IWYU pragma: keep +#include "core/def.hpp" +#include "core/debug.hpp" +#include "core/logger.hpp" +#include "core/data/vecfield/vecfield.hpp" +#include "core/hybrid/hybrid_quantities.hpp" +#include "core/data/vecfield/vecfield_component.hpp" +#include "core/numerics/interpolator/interpolator.hpp" + +// AMR headers +#include "refiner_pool.hpp" +// ... other AMR headers + +// SAMRAI headers +#include <SAMRAI/hier/IntVector.h> +#include <SAMRAI/hier/VariableDatabase.h> +// ... other SAMRAI headers + +// Standard library +#include <memory> +#include <string> +#include <utility> +#include <iomanip> +#include <iostream> +#include <iterator>src/core/data/grid/gridlayout.hpp (1)
1163-1177: Consider simplifying the lambda initializationThe
AMRGhostBoxForimplementation is correct, but the lambda initialization could be simplified.- auto const growBy = [&]() { - std::array<int, dimension> arr; - for (std::uint8_t i = 0; i < dimension; ++i) - arr[i] = nbrGhosts(centerings[i]); - return arr; - }(); + std::array<int, dimension> growBy; + for (std::uint8_t i = 0; i < dimension; ++i) + growBy[i] = nbrGhosts(centerings[i]);The current implementation is functionally correct, but a direct initialization would be clearer.
tests/simulator/refinement/test_2d_overlaps.py (1)
71-72: Remove or document unused variablesThese variables are computed but never used. Either remove them or add a comment explaining why they're kept (e.g., for future use or documentation).
- dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) - dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) + # Magnetic field perturbations (commented out for testing) + # dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) + # dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) return 0.0 # dBy1 + dBy2And similarly for
dBx1anddBx2.Also applies to: 86-87
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/amr/data/particles/particles_data.hpp(0 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(7 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(5 hunks)src/amr/multiphysics_integrator.hpp(4 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(13 hunks)src/amr/utilities/box/amr_box.hpp(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/debug.cpp(1 hunks)src/core/debug.hpp(1 hunks)src/core/logger.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/point/point.hpp(3 hunks)src/simulator/simulator.hpp(3 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/particles/particles_data.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/amr/resources_manager/resources_manager.hppsrc/simulator/simulator.hppsrc/core/logger.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/solvers/solver_ppc.hppsrc/core/utilities/point/point.hppsrc/amr/multiphysics_integrator.hppsrc/amr/utilities/box/amr_box.hppsrc/core/data/grid/gridlayout.hppsrc/core/utilities/box/box.hppsrc/core/debug.hpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/CMakeLists.txt (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
src/simulator/simulator.hpp (3)
Learnt from: nicolasaunai
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
src/core/logger.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
src/amr/level_initializer/hybrid_level_initializer.hpp (1)
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
src/amr/solvers/solver_ppc.hpp (1)
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
src/core/utilities/point/point.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
src/amr/utilities/box/amr_box.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
src/core/utilities/box/box.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#998
File: src/core/utilities/point/point.hpp:195-210
Timestamp: 2025-04-17T14:41:53.643Z
Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
src/core/debug.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
🧬 Code Graph Analysis (2)
src/core/debug.cpp (1)
src/core/debug.hpp (2)
debug_scope(185-185)debug_scope(187-187)
src/core/data/grid/gridlayout.hpp (3)
src/core/utilities/point/point.hpp (4)
i(68-68)i(68-68)i(69-69)i(69-69)pyphare/pyphare/core/gridlayout.py (1)
nbrGhosts(235-239)src/core/utilities/box/box.hpp (7)
grow(82-87)grow(82-82)grow(267-272)grow(267-267)lower(94-94)lower(99-99)upper(89-89)
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
3-3: os imported but unused
Remove unused import: os
(F401)
10-10: pyphare.pharesee.run.Run imported but unused
Remove unused import: pyphare.pharesee.run.Run
(F401)
58-58: Ambiguous variable name: l
(E741)
71-71: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
72-72: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
86-86: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
87-87: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 3-3: 'os' imported but unused
(F401)
[error] 10-10: 'pyphare.pharesee.run.Run' imported but unused
(F401)
[error] 58-58: ambiguous variable name 'l'
(E741)
[error] 71-71: local variable 'dBy1' is assigned to but never used
(F841)
[error] 72-72: local variable 'dBy2' is assigned to but never used
(F841)
[error] 86-86: local variable 'dBx1' is assigned to but never used
(F841)
[error] 87-87: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 8-8: No name 'pharein' in module 'pyphare'
(E0611)
[error] 9-9: No name 'cpp' in module 'pyphare'
(E0611)
[error] 10-10: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 11-11: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 30-30: Too many local variables (17/15)
(R0914)
[error] 149-149: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 163-163: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 169-169: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (35)
src/core/CMakeLists.txt (1)
57-57: LGTM! Correctly integrates debug.cpp into the build.The addition of
debug.cppto the SOURCES_CPP list properly integrates the new debug framework implementation into the phare_core library build.src/amr/resources_manager/resources_manager.hpp (1)
282-282: LGTM! Clean accessor method for debug framework integration.The
all_resources()method provides appropriate read-only access to the internal resource map, supporting the debug framework's need to inspect registered resources. The const reference return type prevents unintended modifications.src/core/logger.hpp (1)
15-15: LGTM! Logical extension of logging control.The addition of
|| defined(PHARE_FORCE_LOG_LINE)provides fine-grained control over logging macros, consistent with the existing conditional compilation pattern. This supports the debug framework's logging requirements.src/simulator/simulator.hpp (3)
11-11: LGTM! Proper inclusion of debug framework.The addition of
#include "core/debug.hpp"correctly includes the new debug framework headers needed for the debug scope instrumentation.
371-371: LGTM! Appropriate debug scope instrumentation.The
PHARE_DEBUG_SCOPE("Simulator/initialize/")call provides valuable debug tracing for the initialization phase, complementing the existing logging scope.
413-413: LGTM! Consistent debug scope instrumentation.The
PHARE_DEBUG_SCOPE("Simulator/advance/")call maintains consistent debug tracing throughout the simulation lifecycle, parallel to the existing logging infrastructure.src/core/debug.cpp (4)
8-12: LGTM! Thread-safe singleton implementation.The static local variable pattern ensures thread-safe singleton initialization in C++11 and later. This is a clean and efficient implementation.
15-24: LGTM! Correct scope stack management.The constructor properly maintains the linked stack structure by:
- Setting parent to current stack pointer
- Updating parent's child pointer if parent exists
- Updating the global stack pointer to this scope
This ensures proper scope nesting and parent-child relationships.
26-29: LGTM! Proper cleanup in destructor.The destructor correctly restores the stack pointer to the parent, ensuring proper scope unwinding. The RAII pattern guarantees automatic cleanup when scopes exit.
18-24: Consider thread safety for multi-threaded debugging.While the singleton is thread-safe, the stack manipulation (lines 18-23) is not protected by any synchronization mechanism. If debug scopes are used across multiple threads, this could lead to race conditions.
Consider whether thread-local storage or synchronization is needed for the debug stack, depending on your threading model.
Are debug scopes intended to be used in multi-threaded contexts? If so, thread safety measures may be needed.
src/amr/level_initializer/hybrid_level_initializer.hpp (3)
52-55: LGTM: Debug instrumentation follows consistent pattern.The debug macros provide good scoped context with level number identification and hierarchical scope naming.
89-91: Good optimization: Resource manager usage improved.The local alias
rmreduces repetitive dereferencing, and using the resource manager'senumeratemethod provides better encapsulation than direct level iteration.
164-164: LGTM: Debug verification at method end.Adding the level check at the end of initialization provides good runtime verification of the level state consistency.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
305-307: LGTM: Debug instrumentation in regrid method.The debug scope and level check provide good diagnostics for the regriding process, which is a critical operation that benefits from verification.
638-639: LGTM: Debug checks after synchronization operations.Adding debug verification after sync and post-sync operations is appropriate since these operations modify grid layouts and field consistency.
Also applies to: 672-673
src/amr/utilities/box/amr_box.hpp (3)
89-100: LGTM: Well-implemented particle-in-box check.The function correctly uses the particle's dimension and iCell member to check bounds against the SAMRAI box. The template design allows it to work with any particle type that has the required interface.
103-115: LGTM: Clean SAMRAI to PHARE type conversions.The
as_pointoverloads provide a clean interface for converting SAMRAI types to PHARE core points. The use ofcore::for_Nwith array mode is an elegant way to extract components.
118-123: LGTM: Consistent box transformation utility.The shift function provides a clean interface for applying SAMRAI transformations to PHARE boxes by leveraging the existing
as_pointand coreshiftfunctions.src/amr/solvers/solver_ppc.hpp (3)
251-251: LGTM: Comprehensive debug scope coverage.The debug scopes provide good hierarchical tracing for all major solver operations (advanceLevel, predictor1, predictor2, corrector, moveIons).
Also applies to: 286-286, 324-324, 364-364, 449-449
260-260: LGTM: Strategic debug checks after critical operations.The placement of overlap checks and field validation after predictors, corrector, and ion movement provides good verification points for the PPC algorithm.
Also applies to: 314-315, 353-353, 395-395, 475-477
420-441: Good simplification: Focus on essential particle count verification.The simplified debug logging removes unnecessary counters while keeping the essential runtime check for ghost particle consistency. This makes the debug output cleaner and more focused.
src/core/utilities/point/point.hpp (4)
133-159: LGTM: Well-implemented compound assignment operators.The
+=and-=operators support both scalar and array-like container types with appropriate template constraints. The implementation is consistent and follows good practices.
194-208: LGTM: Consistent multiplication operators.The multiplication operators follow the same pattern as existing addition/subtraction operators, providing support for scalar, array, and Point operands.
225-234: Good safety check in as_unsigned conversion.The runtime assertion to verify non-negativity in debug builds is a good safety measure for the unsigned conversion. The compile-time type size check ensures only supported types are handled.
231-234: ```shell
#!/bin/bash
sed -n '200,260p' src/core/utilities/point/point.hpp</details> <details> <summary>src/amr/multiphysics_integrator.hpp (3)</summary> `440-440`: **Good optimization: Using const reference for griddingAlg parameter** This change avoids unnecessary copying of the shared_ptr parameter. --- `506-508`: **Debug instrumentation looks good** The debug context is properly set with time, level number, and hierarchy. The scoped debug tracking with `PHARE_DEBUG_SCOPE` ensures proper cleanup when the scope exits. --- `564-564`: **Good optimization: Using const reference for oldTimes parameter** This change avoids unnecessary copying of the vector parameter. </details> <details> <summary>src/core/data/grid/gridlayout.hpp (2)</summary> `1153-1159`: **Well-structured ghost box computation** The `ghostBoxFor` method provides a clean interface for obtaining the ghost region of a field. The use of the `_BoxFor` helper with a lambda makes the code reusable and easy to understand. --- `1237-1258`: **Good use of constexpr for dimension-specific code** The `_BoxFor` helper method correctly handles 1D, 2D, and 3D cases using `if constexpr`, ensuring compile-time optimization. </details> <details> <summary>src/core/utilities/box/box.hpp (4)</summary> `28-28`: **Good addition of static dimension member** Adding a static dimension member provides compile-time access to the box dimension, which is useful for template metaprogramming. --- `75-87`: **Well-designed grow method overloads** The grow methods now support both scalar (uniform growth) and array (per-dimension growth) parameters, providing flexibility while maintaining backward compatibility. --- `202-227`: **Good consolidation of isIn functionality** The new isIn overloads provide a clean, generic interface for checking containment of particles and points within boxes. The SFINAE-based particle overload ensures it only matches types with an `iCell` member. --- `274-281`: **Useful shift utility function** The shift function provides a clean way to translate boxes, which is essential for periodic boundary conditions and coordinate transformations. </details> <details> <summary>src/core/debug.hpp (1)</summary> `11-11`: **Clarify serial operation limitation vs MPI usage** The comment states "This is only expected to work for serial operations!" but the test file `test_2d_overlaps.py` uses MPI. This inconsistency should be clarified. Does this debugging framework support MPI parallel execution? The test seems to use MPI rank checks: ```python if cpp.mpi_rank() == 0: L0L1_datahier = get_hier(diag_dir) test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/debug.hpp (1)
411-411: Fix undefined variable reference.The variable
timeis undefined in this scope and should referencedebugger.time.- PHARE_LOG_LINE_SS("fail " << path << " " << f0->field.name() << " time " << time); + PHARE_LOG_LINE_SS("fail " << path << " " << f0->field.name() << " time " << debugger.time);
🧹 Nitpick comments (7)
src/core/debug.hpp (3)
110-120: Consider adding parameter validation and documentation.The
setmethod usesautoparameters without constraints, which reduces type safety and readability. Consider adding parameter validation or at least documenting the expected types.+ // Expected: h is a hierarchy pointer with domainBox() method void set(double const t, int l, auto h) { + assert(h != nullptr); time = t; level = l;
129-176: Complex template logic in Shifter::make_shift_for.The template function has significant code duplication across different dimensions. Consider extracting common logic or using constexpr if patterns more effectively.
The current implementation repeats similar patterns for 1D, 2D, and 3D cases. A more maintainable approach might use helper functions or constexpr recursion to reduce duplication.
253-254: Improve template constraint expressiveness.The dimension check using
std::decay_tcould be more explicit about the expected interface.Consider using concepts (C++20) or static_assert with clearer error messages:
static_assert(std::decay_t<decltype(dst)>::dimension == DEBUG_DIM, "Field dimension must match DEBUG_DIM for debugging");tests/simulator/refinement/test_2d_overlaps.py (4)
56-56: Use more descriptive variable name.The variable
lis ambiguous and could be confused with the number1.-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, length_scale): + return 0.5 * (1.0 + np.tanh((y - y0) / length_scale))
69-70: Remove unused magnetic field perturbation variables.The calculated perturbation terms are not used since the function returns a constant value.
def by(x, y): Lx = sim.simulation_domain()[0] Ly = sim.simulation_domain()[1] - sigma = 1.0 - dB = 0.1 - - x0 = x - 0.5 * Lx - y1 = y - 0.3 * Ly - y2 = y - 0.7 * Ly - - dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) - dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) return 0.0 # dBy1 + dBy2Similar cleanup needed for the
bxfunction.Also applies to: 84-85
28-141: Consider refactoring the config function for better maintainability.The function has 17 local variables and multiple nested function definitions, making it complex to maintain.
Consider extracting field definition functions to module level or grouping related functionality:
def create_field_functions(sim, L): """Create all field definition functions for the simulation.""" # Move density, S, bx, by, bz, T, velocity functions here # Return them as a dictionary or named tuple pass def config(): L = 0.5 sim = ph.Simulation(...) field_funcs = create_field_functions(sim, L) # Use field_funcs in model creation
166-167: Consider using modern Python super() syntax.For Python 3 compatibility and cleaner code.
- def tearDown(self): - super(HarrisTest, self).tearDown() + def tearDown(self): + super().tearDown()Also applies to the
__init__method at line 161.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare_tests/test_pharesee/test_hierarchy.py(1 hunks)src/core/debug.hpp(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyphare/pyphare_tests/test_pharesee/test_hierarchy.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/debug.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.050Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack managed by the Debuggerino singleton, and the destructor properly restores the stack_ptr to the parent. All memory management is handled through stack lifetime with no dynamic allocation.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.050Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack and the destructor properly restores pointers, with all memory management handled through stack lifetime.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/debug.hpp (8)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.050Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack managed by the Debuggerino singleton, and the destructor properly restores the stack_ptr to the parent. All memory management is handled through stack lifetime with no dynamic allocation.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.050Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack and the destructor properly restores pointers, with all memory management handled through stack lifetime.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
56-56: Ambiguous variable name: l
(E741)
69-69: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
70-70: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
84-84: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
85-85: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 56-56: ambiguous variable name 'l'
(E741)
[error] 69-69: local variable 'dBy1' is assigned to but never used
(F841)
[error] 70-70: local variable 'dBy2' is assigned to but never used
(F841)
[error] 84-84: local variable 'dBx1' is assigned to but never used
(F841)
[error] 85-85: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 7-7: No name 'pharein' in module 'pyphare'
(E0611)
[error] 8-8: No name 'cpp' in module 'pyphare'
(E0611)
[error] 9-9: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 28-28: Too many local variables (17/15)
(R0914)
[error] 147-147: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 161-161: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 167-167: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (1)
tests/simulator/refinement/test_2d_overlaps.py (1)
159-183: Well-structured test class with proper lifecycle management.The test class properly handles simulation initialization, execution, and cleanup with MPI support.
PhilipDeegan
force-pushed
the
debuggerino
branch
from
7f7d751 to
dfcf77b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/simulator/refinement/test_2d_overlaps.py (4)
56-56: Use a more descriptive parameter name.The parameter
lis ambiguous. Consider using a more descriptive name likelength_scaleorscale.- def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) + def S(y, y0, length_scale): + return 0.5 * (1.0 + np.tanh((y - y0) / length_scale))
69-72: Clean up unused perturbation variables.The perturbation terms
dBy1,dBy2,dBx1, anddBx2are computed but not used in the return statements. This creates linting warnings. Consider either removing them or using a clear TODO comment if they're intended for future use.For the
byfunction:def by(x, y): - Lx = sim.simulation_domain()[0] - Ly = sim.simulation_domain()[1] - sigma = 1.0 - dB = 0.1 - - x0 = x - 0.5 * Lx - y1 = y - 0.3 * Ly - y2 = y - 0.7 * Ly - - dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) - dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) - - return 0.0 # dBy1 + dBy2 + # TODO: Add perturbation terms when needed + # Perturbation calculation code removed to avoid unused variable warnings + return 0.0Similar cleanup applies to the
bxfunction fordBx1anddBx2.Also applies to: 84-89
28-141: Consider refactoring the large config() function.The function has 17 local variables and handles both simulation configuration and field function definitions. Consider extracting the field functions into a separate module or class to improve maintainability and readability.
Example approach:
def create_field_functions(sim, L): """Create and return all field function definitions.""" # Move all the nested functions here return { 'density': density, 'bx': bx, 'by': by, 'bz': bz, 'vx': vx, 'vy': vy, 'vz': vz, # etc. } def config(): # Focus only on simulation setup L = 0.5 sim = ph.Simulation(...) fields = create_field_functions(sim, L) model = ph.MaxwellianFluidModel( bx=fields['bx'], by=fields['by'], # etc. )
161-161: Modernize super() calls.Use Python 3 style super() without arguments for cleaner code.
- super(HarrisTest, self).__init__(*args, **kwargs) + super().__init__(*args, **kwargs)- super(HarrisTest, self).tearDown() + super().tearDown()Also applies to: 167-167
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/amr/data/particles/particles_data.hpp(0 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(7 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(5 hunks)src/amr/multiphysics_integrator.hpp(4 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(13 hunks)src/amr/utilities/box/amr_box.hpp(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/debug.cpp(1 hunks)src/core/debug.hpp(1 hunks)src/core/logger.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/point/point.hpp(3 hunks)src/simulator/simulator.hpp(3 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/particles/particles_data.hpp
🚧 Files skipped from review as they are similar to previous changes (14)
- src/core/CMakeLists.txt
- src/core/logger.hpp
- src/amr/resources_manager/resources_manager.hpp
- src/simulator/simulator.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/core/debug.cpp
- src/amr/solvers/solver_ppc.hpp
- src/amr/utilities/box/amr_box.hpp
- src/core/data/grid/gridlayout.hpp
- src/core/utilities/point/point.hpp
- src/amr/multiphysics_integrator.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/core/utilities/box/box.hpp
- src/core/debug.hpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
56-56: Ambiguous variable name: l
(E741)
69-69: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
70-70: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
84-84: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
85-85: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 56-56: ambiguous variable name 'l'
(E741)
[error] 69-69: local variable 'dBy1' is assigned to but never used
(F841)
[error] 70-70: local variable 'dBy2' is assigned to but never used
(F841)
[error] 84-84: local variable 'dBx1' is assigned to but never used
(F841)
[error] 85-85: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 7-7: No name 'pharein' in module 'pyphare'
(E0611)
[error] 8-8: No name 'cpp' in module 'pyphare'
(E0611)
[error] 9-9: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 28-28: Too many local variables (17/15)
(R0914)
[error] 147-147: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 161-161: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 167-167: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
tests/simulator/refinement/test_2d_overlaps.py (3)
1-26: Setup looks good.The imports and global configuration are well-structured and appropriate for the 2D refinement test.
144-157: Utility functions look good.The data loading functions are clean and serve their purpose well.
186-188: Main execution looks good.Clean and appropriate test execution with proper MPI initialization.
PhilipDeegan
force-pushed
the
debuggerino
branch
from
dfcf77b to
a0388de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/debug.hpp (2)
110-120: Consider adding parameter validation.The
setmethod accepts a hierarchy parameterhwithout validation. Consider adding a null check to prevent potential segmentation faults.void set(double const t, int l, auto h) { + if (!h) return; time = t; level = l; auto domainBoxVec = h->domainBox();
270-274: Consider performance optimization for active field checking.The nested loop structure for checking active debug fields could be expensive if called frequently. Consider using a hash map or set for faster lookups, especially for the path and field name comparisons.
for (auto [k, vs] : debugging) for (auto const& v : vs) - if (float_equals(debugger.time, v.time) and path == k - and v.dst_name == dst.field.name() and v.box * dst_amr_box) + if (k == path && v.dst_name == dst.field.name() && + float_equals(debugger.time, v.time) && v.box * dst_amr_box) debug_fields(dst, src, k, v);Reordering the conditions to check exact matches first before the more expensive
float_equalscall.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/amr/level_initializer/hybrid_level_initializer.hpp(3 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(5 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(12 hunks)src/amr/utilities/box/amr_box.hpp(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/debug.cpp(1 hunks)src/core/debug.hpp(1 hunks)src/core/logger.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/point/point.hpp(3 hunks)src/simulator/simulator.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/core/logger.hpp
- src/amr/resources_manager/resources_manager.hpp
- src/core/CMakeLists.txt
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/simulator/simulator.hpp
- src/core/debug.cpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/amr/utilities/box/amr_box.hpp
- src/amr/solvers/solver_ppc.hpp
- src/amr/multiphysics_integrator.hpp
- src/core/utilities/point/point.hpp
- src/core/utilities/box/box.hpp
- src/core/data/grid/gridlayout.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/debug.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/debug.hpp (8)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.078Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack managed by the Debuggerino singleton, and the destructor properly restores the stack_ptr to the parent. All memory management is handled through stack lifetime with no dynamic allocation.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.078Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack and the destructor properly restores pointers, with all memory management handled through stack lifetime.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
🔇 Additional comments (5)
src/core/debug.hpp (5)
129-176: Excellent implementation of periodic boundary shift calculation.The
Shifterstruct provides a well-structured approach to computing periodic shifts for different dimensionalities. The template-based design with constexpr branching is efficient and the logic for calculating mid-points and shifts is sound.
183-215: RAII-based debug scope implementation is well-designed.Based on the implementation in
debug.cpp, this debug_scope struct uses stack-allocated objects with proper RAII semantics. The raw pointers are safely managed through object lifetime on the C++ stack with no dynamic allocation.
344-445: Robust overlap checking implementation.The
check_fieldsandcheck_all_fieldsfunctions provide comprehensive validation of field consistency across both periodic and non-periodic boundaries. The implementation correctly handles:
- Active field filtering
- Periodic boundary shifts using the Shifter
- Precise floating-point comparisons
- Detailed logging of mismatches
The nested iteration approach is appropriate for the debugging context where correctness is prioritized over performance.
448-471: Template design follows good C++ practices.The
check_levelfunction template with SFINAE-style dimension checking usingif constexpris well-designed. The integration with SAMRAI patch hierarchy is clean and the type constraints ensure compilation safety.
475-485: Clean macro fallback implementation.The conditional macro definitions provide clean no-op alternatives when debugging is disabled, ensuring zero runtime overhead in release builds while maintaining API compatibility.
| { | ||
| double time; | ||
| Box<int, DEBUG_DIM> box; | ||
| std::string dst_name, src_name = dst_name; |
There was a problem hiding this comment.
Fix member initialization order issue.
The src_name member is initialized with dst_name, but dst_name is uninitialized at the point of src_name's initialization, resulting in undefined behavior.
- std::string dst_name, src_name = dst_name;
+ std::string dst_name, src_name;Or if you intend src_name to default to the same value as dst_name, initialize it in the constructor or use a default member initializer approach.
📝 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.
| std::string dst_name, src_name = dst_name; | |
| std::string dst_name, src_name; |
🤖 Prompt for AI Agents
In src/core/debug.hpp at line 75, the member initialization order is incorrect
because src_name is initialized with dst_name before dst_name is initialized,
causing undefined behavior. To fix this, remove the inline initialization of
src_name with dst_name and instead initialize src_name in the constructor after
dst_name has been initialized, or use a default member initializer for dst_name
and then initialize src_name accordingly in the constructor.
PhilipDeegan
force-pushed
the
debuggerino
branch
from
a0388de to
f64b098
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/debug.hpp (1)
75-75: Fix member initialization order issue.The
src_namemember is initialized withdst_name, butdst_nameis uninitialized at the point ofsrc_name's initialization, resulting in undefined behavior.- std::string dst_name, src_name = dst_name; + std::string dst_name, src_name;
🧹 Nitpick comments (3)
src/core/debug.hpp (3)
127-180: Add documentation for the shift calculation algorithm.The Shifter class implements complex periodic shift logic that would benefit from documentation explaining the mathematical approach for different dimensions.
+ // Generates periodic shifts for domain boundary conditions + // Returns an array of shift vectors for checking periodic overlaps struct Shifter { template<typename T, std::size_t D> auto make_shift_for(PHARE::core::Box<T, D> const& box) const
238-241: Use logger instead of direct cout for consistency.The debug output uses
std::coutdirectly, which is inconsistent with the logging framework used elsewhere in the codebase.- std::cout << "amr_idx=" << amr_idx << " dst(" << dv << ") = src(" << sv << ")" - << std::endl; + PHARE_LOG_LINE_SS("amr_idx=" << amr_idx << " dst(" << dv << ") = src(" << sv << ")");
343-419: Consider refactoring for better maintainability.The
check_fieldsfunction is quite long and contains similar logic for periodic and non-periodic checks that could be extracted into helper functions.+ auto check_overlap = [&](auto const& overlap, auto const& offset = Point<int, DEBUG_DIM>{}) { + auto const lb0 = layout0.AMRToLocal(shift(overlap, offset * -1)); + auto const lb1 = layout1.AMRToLocal(overlap); + // ... overlap checking logic + }; + // periodic checks for (auto const& offset : shifter.make_shift_for(layout0.AMRBox())) { if (auto const overlap_ = shift(gb0, offset) * gb1) - // ... duplicate logic + check_overlap(*overlap_, offset); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/amr/level_initializer/hybrid_level_initializer.hpp(3 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(5 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(12 hunks)src/amr/utilities/box/amr_box.hpp(1 hunks)src/core/CMakeLists.txt(1 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/debug.cpp(1 hunks)src/core/debug.hpp(1 hunks)src/core/logger.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/point/point.hpp(3 hunks)src/simulator/simulator.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/core/CMakeLists.txt
- src/core/logger.hpp
- src/amr/resources_manager/resources_manager.hpp
- src/simulator/simulator.hpp
- src/core/debug.cpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/utilities/box/amr_box.hpp
- src/amr/solvers/solver_ppc.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/core/data/grid/gridlayout.hpp
- src/amr/multiphysics_integrator.hpp
- src/core/utilities/point/point.hpp
- src/core/utilities/box/box.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/debug.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
src/core/debug.hpp (8)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.078Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack managed by the Debuggerino singleton, and the destructor properly restores the stack_ptr to the parent. All memory management is handled through stack lifetime with no dynamic allocation.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#1032
File: src/core/debug.hpp:183-215
Timestamp: 2025-06-27T10:59:18.078Z
Learning: In the PHARE project, the debug_scope struct in src/core/debug.hpp uses raw pointers for parent/child relationships but is safe because debug_scope objects are stack-allocated with RAII semantics. The constructor links into a scope stack and the destructor properly restores pointers, with all memory management handled through stack lifetime.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.609Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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
PR: PHAREHUB/PHARE#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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
🔇 Additional comments (4)
src/core/debug.hpp (4)
1-65: Well-structured debug framework setup.The conditional compilation approach and macro definitions provide a clean interface for enabling/disabling debug features.
104-126: Singleton pattern implementation is appropriate.The Debuggerino singleton provides a clean global state management for debugging context.
183-215: Raw pointer usage is safe with RAII semantics.Based on previous discussion and analysis of debug.cpp, the raw pointers are safely managed through stack allocation with proper RAII cleanup.
448-488: Clean conditional compilation and template usage.The explicit template parameter for
check_leveland proper macro disabling provide good performance characteristics when debugging is disabled.
|
currently this just prints on mismatch, but it could error/throw in a strict mode |
1 participant
means to check ghostbox overlaps, and interrogate field index set/add operations for
class FieldBox