Conversation
📝 WalkthroughWalkthroughThis PR removes the origin concept from the coordinate system and refactors GridLayout to use AMR-based local indexing. New methods for AMR-local conversions are added, coordinate calculation APIs are simplified to eliminate origin-based offsets, and test patterns are updated to use unified AMR-local iteration instead of dimension-specific nested loops. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant GL as GridLayout
participant AMRIdx as AMR Indexing
participant LocalIdx as Local Indexing
User->>GL: Request field node coordinates
GL->>AMRIdx: Convert to AMR Point
AMRIdx-->>GL: AMR Point (no origin offset)
GL->>LocalIdx: Get local indices
LocalIdx-->>GL: Local index tuple
GL-->>User: Position (computed from AMR Point)
Note over GL,LocalIdx: Old flow: origin + startIndex + delta<br/>New flow: AMR point directly
sequenceDiagram
participant OldCode as Old Approach
participant NewCode as New Approach
OldCode->>OldCode: for each dimension i:<br/> pos[i] = origin[i] + (iCell[i] - startIdx[i]) * meshSize[i]
NewCode->>NewCode: pos = (iCell + delta) * meshSize<br/>(origin = 0.0 by default)
Note over OldCode: Origin-based coordinate offset
Note over NewCode: Simplified AMR-local coordinates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.hpp⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-04-17T14:41:53.643ZApplied to files:
📚 Learning: 2024-09-17T13:33:47.475ZApplied to files:
📚 Learning: 2025-09-17T13:35:11.533ZApplied to files:
📚 Learning: 2025-09-17T13:50:16.038ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PhilipDeegan
force-pushed
the
omg
branch
6 times, most recently
from
dee9ffa to
ee2989c
Compare
![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
omg
branch
2 times, most recently
from
954f79d to
5ae6ae1
Compare
|
It is possible that the rounding error we fixed with this PR was responsible for the remaining mismatches identified in #1082 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
70-72: Verified: inconsistent lower bound check is a real issue requiring correction.The
positionAsPointimplementation confirms that coordinates are computed as(iCell + delta) * meshSize, whereiCellis a global AMR index. This means particles in the AMR box range [50, 99] with meshSize 0.1 will have positions in [5.0, 10.0]. The hardcoded lower bound of 0.0 is inconsistent with the upper bound calculation and incorrectly permits invalid particles.The suggested fix is correct—apply the diff to use AMR-based bounds consistently on both sides.
src/core/data/grid/gridlayout.hpp (1)
454-529: Restore origin offset when computing coordinatesRemoving
origin_fromfieldNodeCoordinatesandcellCenteredCoordinatesnow anchors every layout at 0, so any non-zero domain origin yields wrong physical positions for fields and cells. That breaks user configurations relying on translated meshes and can propagate errors into initialization and diagnostics. Please add the origin offset back into both helpers.- position[iDir] = (static_cast<double>(amr_idx[iDir]) + halfCell) * meshSize_[iDir]; + position[iDir] = origin_[iDir] + + (static_cast<double>(amr_idx[iDir]) + halfCell) * meshSize_[iDir]; @@ - physicalPosition[iDir] - = (static_cast<double>(coord[iDir]) + halfCell) * meshSize_[iDir]; + physicalPosition[iDir] + = origin_[iDir] + + (static_cast<double>(coord[iDir]) + halfCell) * meshSize_[iDir];
🧹 Nitpick comments (2)
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
77-77: Remove unused local variable.The
layoutvariable is declared but never used. Remove it to keep the code minimal and efficient.Apply this diff:
- params.back().base = createParam<GridLayoutImpl>(dl, nbCell, Point{origin}); - auto const& layout = *params.back().base.layout; + params.back().base = createParam<GridLayoutImpl>(dl, nbCell, Point{origin});Based on learnings
src/diagnostic/diagnostic_model_view.hpp (1)
103-103: Clarify empty vector initialization.The expression
std::vector<double>(0, dimension)creates an empty vector (size 0) but is confusing because it appears to passdimensionas a value parameter that is never used. For clarity, usestd::vector<double>{}orstd::vector<double>(0)instead. If the intent is to return a zero-filled vector of sizedimension, usestd::vector<double>(dimension, 0.0).Apply this diff for clarity:
- NO_DISCARD auto origin() const { return std::vector<double>(0, dimension); } + NO_DISCARD auto origin() const { return std::vector<double>{}; }Or if a zero-filled vector is intended:
- NO_DISCARD auto origin() const { return std::vector<double>(0, dimension); } + NO_DISCARD auto origin() const { return std::vector<double>(dimension, 0.0); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
pyphare/pyphare/core/gridlayout.py(1 hunks)pyphare/pyphare/pharein/__init__.py(0 hunks)pyphare/pyphare/pharein/maxwellian_fluid_model.py(2 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)src/amr/resources_manager/amr_utils.hpp(2 hunks)src/amr/wrappers/hierarchy.hpp(2 hunks)src/core/data/field/initializers/field_user_initializer.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(10 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp(2 hunks)src/core/data/particles/particle_utilities.hpp(1 hunks)src/core/utilities/box/box.hpp(1 hunks)src/core/utilities/point/point.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(1 hunks)tests/amr/data/field/copy_pack/field_data_test_param.hpp(2 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp(1 hunks)tests/amr/data/field/refine/test_tag_strategy.hpp(1 hunks)tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp(4 hunks)tests/amr/messengers/test_messenger_tag_strategy.hpp(1 hunks)tests/core/data/electrons/test_electrons.cpp(4 hunks)tests/core/data/gridlayout/cellCenteredCoordinates.py(6 hunks)tests/core/data/gridlayout/deriv.py(3 hunks)tests/core/data/gridlayout/fieldNodeCoordinates.py(5 hunks)tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp(3 hunks)tests/core/data/gridlayout/gridlayout_deriv.cpp(5 hunks)tests/core/data/gridlayout/gridlayout_deriv.hpp(1 hunks)tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp(5 hunks)tests/core/data/gridlayout/gridlayout_laplacian.cpp(9 hunks)tests/core/data/gridlayout/test_main.cpp(1 hunks)tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp(1 hunks)tests/core/data/particles/test_main.cpp(1 hunks)tests/core/data/particles/test_particle_array_consistency.cpp(1 hunks)tests/core/numerics/ampere/test_main.cpp(10 hunks)tests/core/numerics/faraday/test_main.cpp(17 hunks)tests/core/numerics/ion_updater/test_updater.cpp(1 hunks)tests/core/numerics/ohm/test_main.cpp(10 hunks)tests/initializer/test_initializer.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- pyphare/pyphare/pharein/init.py
- tests/initializer/test_initializer.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/core/utilities/types.hppsrc/core/data/particles/particle_utilities.hppsrc/core/utilities/box/box.hpptests/amr/data/field/copy_pack/field_data_test_param.hppsrc/core/data/ions/particle_initializers/maxwellian_particle_initializer.hppsrc/core/data/field/initializers/field_user_initializer.hppsrc/core/utilities/point/point.hpptests/amr/messengers/test_messenger_tag_strategy.hppsrc/amr/resources_manager/amr_utils.hppsrc/diagnostic/diagnostic_model_view.hpptests/amr/data/field/refine/test_tag_strategy.hpptests/core/data/gridlayout/gridlayout_cell_centered_coord.hppsrc/amr/wrappers/hierarchy.hpptests/core/data/gridlayout/gridlayout_deriv.hpptests/core/data/gridlayout/gridlayout_field_centered_coord.hppsrc/core/data/grid/gridlayout.hpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-09-01T09:41:40.618Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1056
File: tests/core/numerics/interpolator/test_main.cpp:314-318
Timestamp: 2025-09-01T09:41:40.618Z
Learning: The PHARE project uses C++20, which makes the typename keyword optional for dependent types in obvious contexts. The project prefers minimal code without unnecessary keywords, so typename suggestions for dependent types should be avoided when the code compiles successfully in C++20.
Applied to files:
src/core/utilities/types.hpptests/core/data/particles/test_particle_array_consistency.cpptests/core/data/electrons/test_electrons.cpp
📚 Learning: 2025-02-06T10:52:56.461Z
Learnt from: rochSmets
Repo: PHAREHUB/PHARE PR: 921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
Applied to files:
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpptests/core/data/particles/test_particle_array_consistency.cpp
📚 Learning: 2025-04-17T14:41:53.643Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 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.
Applied to files:
src/core/data/field/initializers/field_user_initializer.hppsrc/core/utilities/point/point.hppsrc/amr/resources_manager/amr_utils.hpp
🧬 Code graph analysis (20)
src/core/data/particles/particle_utilities.hpp (2)
tests/amr/data/field/refine/test_tag_strategy.hpp (2)
position(205-227)position(205-205)src/core/data/particles/particle_packer.hpp (1)
particle(27-27)
tests/core/data/gridlayout/cellCenteredCoordinates.py (1)
pyphare/pyphare/core/gridlayout.py (4)
localPointToAMR(314-317)physicalStartIndex(262-264)physicalEndIndex(266-272)isDual(247-251)
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
src/core/data/grid/gridlayout.hpp (2)
indices(232-239)indices(232-232)src/core/utilities/types.hpp (2)
for_N_make_array(486-489)for_N_make_array(486-486)
src/core/data/field/initializers/field_user_initializer.hpp (2)
src/core/data/grid/gridlayout.hpp (2)
indices(232-239)indices(232-232)tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
field(24-37)
tests/amr/messengers/test_messenger_tag_strategy.hpp (1)
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
field(24-37)
pyphare/pyphare/core/gridlayout.py (4)
src/core/data/particles/particle_array.hpp (2)
box(232-232)box(232-232)src/core/utilities/cellmap.hpp (4)
box(189-189)box(189-189)box(190-190)box(190-190)src/core/data/grid/gridlayout.hpp (4)
physicalStartIndex(316-319)physicalStartIndex(316-316)physicalStartIndex(322-326)physicalStartIndex(322-322)pyphare/pyphare/simulator/simulator.py (1)
interp_order(283-285)
tests/core/data/gridlayout/fieldNodeCoordinates.py (1)
pyphare/pyphare/core/gridlayout.py (3)
fieldCoords(402-414)qtyCentering(224-225)GridLayout(168-437)
src/amr/resources_manager/amr_utils.hpp (2)
src/core/utilities/types.hpp (2)
for_N_make_array(486-489)for_N_make_array(486-486)src/amr/data/field/field_data_factory.hpp (2)
patch(65-78)patch(66-66)
src/diagnostic/diagnostic_model_view.hpp (3)
src/core/utilities/point/point.hpp (1)
vector(101-104)src/amr/resources_manager/resources_manager.hpp (1)
dimension(85-294)src/core/data/grid/gridlayoutimplyee.hpp (3)
dimension(994-1016)dimension(1020-1042)dimension(1046-1068)
tests/core/data/particles/test_main.cpp (1)
src/core/data/particles/particle_utilities.hpp (2)
positionAsPoint(19-29)positionAsPoint(19-20)
tests/core/data/gridlayout/gridlayout_deriv.cpp (4)
tests/core/data/gridlayout/gridlayout_deriv.hpp (1)
read(20-20)tests/core/numerics/ampere/test_main.cpp (2)
read(110-119)read(110-110)tests/core/numerics/faraday/test_main.cpp (2)
read(124-133)read(124-124)tests/core/numerics/ohm/test_main.cpp (2)
read(326-335)read(326-326)
tests/core/data/gridlayout/deriv.py (1)
pyphare/pyphare/core/gridlayout.py (1)
GridLayout(168-437)
pyphare/pyphare/pharein/maxwellian_fluid_model.py (2)
pyphare/pyphare/core/gridlayout.py (1)
GridLayout(168-437)pyphare/pyphare/simulator/simulator.py (2)
domain_box(275-277)interp_order(283-285)
tests/amr/data/field/refine/test_tag_strategy.hpp (2)
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
field(24-37)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
field(929-956)field(929-930)field(958-964)field(958-958)
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
tests/core/data/gridlayout/gridlayout_params.hpp (2)
createParam(54-64)createParam(54-56)
tests/core/data/particles/test_particle_array_consistency.cpp (1)
src/core/data/particles/particle_array.hpp (10)
box(232-232)box(232-232)i(75-75)i(75-75)i(76-76)i(76-76)i(305-305)i(305-305)i(306-306)i(306-306)
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (2)
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
field(27-40)tests/core/data/gridlayout/gridlayout_params.hpp (2)
createParam(54-64)createParam(54-56)
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (3)
tests/amr/data/field/refine/test_tag_strategy.hpp (4)
hierarchy(146-197)hierarchy(146-151)position(205-227)position(205-205)tests/amr/resources_manager/test_resources_manager_basic_hierarchy.hpp (2)
init(47-79)init(47-47)src/core/data/field/initializers/field_user_initializer.hpp (2)
initialize(18-35)initialize(18-19)
tests/core/data/electrons/test_electrons.cpp (3)
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
field(24-37)tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
field(27-40)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
field(929-956)field(929-930)field(958-964)field(958-958)
src/core/data/grid/gridlayout.hpp (5)
src/core/utilities/box/box.hpp (7)
box(51-51)grow(89-94)grow(89-89)grow(272-277)grow(272-272)that(69-72)that(69-69)src/amr/data/field/field_geometry.hpp (4)
box(136-179)box(136-137)box(198-207)box(198-198)src/core/utilities/types.hpp (19)
generate(273-284)generate(273-273)generate(287-290)generate(287-287)generate(294-304)generate(294-294)generate(307-310)generate(307-307)generate(326-329)generate(326-326)for_N(445-476)for_N(445-445)for_N(479-482)for_N(479-479)auto(151-156)auto(492-495)auto(498-501)for_N_make_array(486-489)for_N_make_array(486-486)src/core/utilities/point/point.hpp (5)
i(68-68)i(68-68)i(69-69)i(69-69)auto(211-211)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
field(929-956)field(929-930)field(958-964)field(958-958)
🪛 Ruff (0.14.3)
pyphare/pyphare/pharein/simulation.py
968-968: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
🔇 Additional comments (58)
tests/core/data/particles/test_particle_array_consistency.cpp (1)
30-32: LGTM! Variable rename improves clarity, dereferencing is safe.The rename from
bixtoamr_idxbetter reflects the PR's shift to AMR-index-based coordinates. The box iterator'soperator*()returns a direct reference to itsindex_member variable, making the dereferencing inherently safe—no null pointer or invalidation risks.tests/core/data/particles/test_main.cpp (1)
83-83: LGTM! Simplified to use AMR indices directly.The direct use of
part.iCellaligns with the PR's objective of using AMR indices instead of per-patch coordinate transformations. This simplification is correct and consistent with the newpositionAsPointutility function.tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (2)
68-68: LGTM! Correctly calculates upper boundary using AMR indices.The updated calculation properly computes the physical coordinate of the domain's upper boundary using the AMR index directly, eliminating the dependency on per-patch origins. This aligns with the PR's objective.
31-31: ****The origin parameter is not deprecated. The GridLayout constructor still requires the origin parameter as its third argument, and it is actively stored in the member initializer list. The test setup at line 31 is correct as-is. This PR changes AMR-based calculations but does not modify the constructor signature or remove the origin parameter.
Likely an incorrect or invalid review comment.
src/core/utilities/box/box.hpp (1)
28-28: LGTM!The type alias relocation improves code organization by placing the member type definition early in the struct, following common C++ conventions.
pyphare/pyphare/pharein/simulation.py (1)
968-968: LGTM!The domain calculation now correctly removes origin-based offsets, aligning with the PR's goal of using AMR-based coordinates. The
zip()usage is safe here sinceself.dlandself.cellsare validated to have equal length bycheck_domain()earlier.src/core/data/particles/particle_utilities.hpp (1)
19-29: LGTM!The simplified coordinate computation correctly uses AMR index space directly with mesh size, eliminating origin-based offsets. The function documentation clearly states the AMR index space assumption, which is essential for correctness.
src/core/utilities/point/point.hpp (1)
221-241: LGTM!The new templated
as<To>()method provides a clean, reusable conversion mechanism. The updatedas_unsigned()andas_signed()methods now leverage this helper, improving code maintainability while preserving the intentional compile-time enforcement for unsupported types.src/core/data/field/initializers/field_user_initializer.hpp (2)
6-10: LGTM!The added includes support the new AMR-local indexing pattern used in the field initialization.
21-34: LGTM!The refactored initialization correctly adopts AMR-aware local indexing:
- Uses
AMRGhostBoxFor(field)to obtain the AMR ghost box- Computes coordinates without origin offsets via
fieldNodeCoordinates(field_, args...)- Transforms AMR indices to local indices via
AMRToLocal(Point{args...})This aligns with the PR's goal of eliminating origin-based coordinate calculations.
tests/core/numerics/faraday/test_main.cpp (2)
30-30: LGTM!The
constqualifier reordering improves consistency with modern C++ style conventions.Also applies to: 44-44, 58-58
237-242: LGTM!The test updates correctly adapt to the new AMR-space coordinate API by converting local indices to AMR space via
layout.localToAMR(Point{...}.as_signed())before passing tofieldNodeCoordinates(). This pattern is consistent across all dimensions (1D, 2D, 3D) and aligns with the PR's objective to use AMR-based indexing.Also applies to: 249-254, 299-303, 310-314, 321-325, 332-336, 343-347, 354-358, 444-449, 460-465, 476-481, 492-496, 507-511, 522-526
tests/amr/data/field/refine/test_tag_strategy.hpp (1)
180-186: LGTM!The refactored field initialization elegantly consolidates dimension-specific logic into a unified AMR-aware pattern:
- Iterates over
AMRGhostBoxFor(field)to obtain AMR indices- Computes coordinates via
fieldNodeCoordinates(field, amr_idx)without origin- Transforms to local indices via
AMRToLocal(amr_idx)This simplification improves maintainability and aligns with the PR's AMR-local indexing approach.
tests/core/data/electrons/test_electrons.cpp (3)
26-26: LGTM!The
constqualifier reordering improves consistency with modern C++ style.
228-244: LGTM!The 1D field initialization correctly adopts the AMR-local indexing pattern using
layout.ghost_amr_lcl_idx(field), which elegantly provides both AMR and local indices in a single iteration. Coordinates are computed viafieldNodeCoordinates(field, amr_idx)without origin offsets.
248-266: LGTM!The 2D and 3D field initialization correctly adopts the AMR-local indexing pattern:
- Iterates over
AMRGhostBoxFor(field)to obtain AMR indices- Computes coordinates via
fieldNodeCoordinates(field, amr_idx)without origin- Transforms to local indices via
AMRToLocal(amr_idx)for field accessThis is consistent with the PR's shift to AMR-based coordinate handling.
Also applies to: 269-300
tests/core/data/gridlayout/cellCenteredCoordinates.py (4)
33-41: LGTM! Good defensive coding with assertions.The transformation to AMR-local indexing is correct, and the assertions validating non-negative indices and proper ordering are helpful safeguards.
44-58: LGTM! Consistent AMR-local indexing for 2D.The separate computation for X and Y dimensions using
localPointToAMRis correct and aligns with the PR's objective of removing origin-based coordinate handling.
95-98: LGTM! Simplified coordinate calculation aligns with PR objectives.The refactored signature and implementation correctly compute cell-centered coordinates using AMR-local indices, eliminating the need for origin and startIndex parameters.
156-156: No issues found with GridLayout instantiation.The GridLayout is intentionally instantiated with only
interp_orderfor this test utility script. The coordinate transformation methods (localPointToAMR,physicalStartIndex, etc.) depend oninterp_orderandcentering, not on theboxordlparameters. The defaultBox(0, 0)contributesbox.lower = 0to the calculation, which does not affect the results since it's added and then effectively neutralized. The actual grid parameters (dl,nbrCell) are properly stored in the separateCenteredCoordParamsobject. This pattern is consistent with similar coordinate generation scripts in the codebase (deriv.py, fieldNodeCoordinates.py).pyphare/pyphare/core/gridlayout.py (1)
314-317: LGTM! Correct local-to-AMR coordinate transformation.The
localPointToAMRmethod properly converts local patch coordinates to AMR global coordinates by adding the box's lower bound and subtracting the local physical start index. This is the inverse ofAMRPointToLocaland aligns with the PR's AMR-aware indexing approach.pyphare/pyphare/pharein/maxwellian_fluid_model.py (2)
199-199: Verify zero-origin assumption is correct for all validation scenarios.The GridLayout now uses
domain_box.lower(which is[0]for 1D) instead ofsim.origin. This assumes the validation domain always starts at zero, which aligns with the PR's goal of removing per-patch origins. Ensure this assumption holds for all simulation configurations.
239-239: Consistent with validate1d changes.The same zero-origin pattern is applied for 2D validation, maintaining consistency with the validate1d changes.
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
21-21: LGTM! Point type is more semantically appropriate.The change from
std::array<std::uint32_t, ...>toPoint<int, ...>improves semantic clarity for coordinate/index representation and aligns with the Point-based coordinate system introduced in this PR.src/amr/wrappers/hierarchy.hpp (2)
266-268: Hardcoded zero-origin assumption is intentional.The domain lower bound is now always set to 0, eliminating the need for origin configuration. This is a fundamental change aligning with the PR's goal of using AMR indices with constant mesh sizes. The upper bound is correctly computed as
lower + nbr_cells * mesh_size.
97-97: The removal of hierarchy.origin() is clean and complete.Verification confirms no remaining calls to
hierarchy.origin()exist in the codebase. The broader references to "origin" are for other classes (GridLayout, diagnostic_model_view, PatchData) and are unrelated to the hierarchy class. The breaking API change has been properly handled.tests/core/data/gridlayout/test_main.cpp (1)
2-6: LGTM! Good cleanup of unused includes.Removing unused includes reduces dependencies and improves compilation time. The test main now only includes what it needs.
Based on learnings
tests/amr/messengers/test_messenger_tag_strategy.hpp (1)
32-33: LGTM! Simplified coordinate query aligns with PR objectives.The removal of the
originparameter fromfieldNodeCoordinatessimplifies the API and is consistent with the PR's shift to AMR-local indexing. The coordinate calculation now relies solely on the field and index.src/core/utilities/types.hpp (1)
485-489: LGTM! Clean convenience wrapper.The
for_N_make_arrayhelper provides a cleaner API for the common pattern of callingfor_Nwithmake_arraymode, reducing boilerplate across the codebase.tests/amr/data/field/copy_pack/field_data_test_param.hpp (2)
89-97: LGTM! Coordinate calculation simplified.The change to use
fieldNodeCoordinates(field, iCell)directly eliminates the origin parameter, aligning with the AMR-local indexing approach introduced across the PR.
104-112: LGTM! Consistent with sourceFill changes.The destination fill logic mirrors the source fill simplification, removing origin-based coordinate handling.
tests/core/data/gridlayout/gridlayout_laplacian.cpp (2)
34-36: LGTM! AMR-local coordinate transformation applied.The updated call wraps the point indices with
localToAMR, ensuring coordinates are computed in the AMR coordinate system rather than raw local indices.
63-65: LGTM! Consistent AMR-local coordinate transformations.All
fieldNodeCoordinatescalls have been consistently updated to uselayout.localToAMR(Point{...}.as_signed()), ensuring uniform AMR-aware coordinate handling across 1D, 2D, and 3D test cases.Also applies to: 92-94, 137-139, 180-182, 223-225, 284-286, 339-341, 394-396
tests/core/numerics/ohm/test_main.cpp (1)
103-104: LGTM! AMR-local coordinate system consistently applied.All
fieldNodeCoordinatescalls have been systematically updated to uselayout.localToAMR(Point{...}.as_signed())across 1D, 2D, and 3D test setup, ensuring uniform AMR-aware indexing throughout the OhmTest fixture.Also applies to: 118-119, 142-143, 154-156, 165-167, 173-175, 202-203, 218-219, 229-230, 237-238, 251-252, 259-260, 270-271
tests/core/numerics/ion_updater/test_updater.cpp (1)
622-628: LGTM! AMR-local iteration improves correctness.The switch to
layout.amr_lcl_idx()iteration ensures the density check uses only complete interpolation domains, avoiding the incomplete last primal cell. Usingamr_idxfor coordinates andlcl_idxfor array indexing is the correct pattern.tests/core/data/gridlayout/fieldNodeCoordinates.py (3)
24-30: LGTM! Signature simplified to remove origin dependency.The
fieldCoordsfunction now takes aGridLayoutinstance (gl) directly and removes thestartIndexandoriginparameters. The coordinate calculation is simplified to(primalIndex + halfCell) * dl, relying on theGridLayoutfor centering information. This aligns with the PR's goal of eliminating origin-based coordinate calculations.
101-101: LGTM! GridLayout instantiated per interpolation order.Creating a
GridLayoutinstance per interpolation order and passing it tofieldCoordsis the correct pattern for the updated API.
152-153: LGTM! All call sites updated consistently.All
fieldCoordscalls have been updated to pass theglparameter and remove the obsoletestartIndexandoriginarguments, ensuring consistent usage across 1D, 2D, and 3D test cases.Also applies to: 164-166, 185-192
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
141-147: LGTM! Generic dimension handling eliminates branches.The
icellanddeltashelpers usingfor_N_make_arrayprovide clean, dimension-agnostic implementations that replace explicit 1D/2D/3D branches. This improves maintainability and aligns with the minimal, efficient code style preferred in this codebase.Based on learnings
153-153: LGTM! AMR box indexing replaces physical domain iteration.Switching from
physicalStartToEndIndicestolayout.indices(layout.AMRBox())aligns with the AMR-local indexing approach introduced across the PR, ensuring consistent coordinate handling.tests/core/data/gridlayout/gridlayout_deriv.hpp (1)
28-28: LGTM! Type alias improves API accessibility.Adding the public
GridLayout_talias exposes the layout type to external code without changing any behavior. This is a clean refactor that improves the test fixture API.Also applies to: 50-50, 72-72
src/amr/resources_manager/amr_utils.hpp (2)
6-6: LGTM: Include reordering for consistency.The include reordering is a minor cleanup with no functional impact.
164-196: Well-structured refactor using generic utilities.The refactored
layoutFromPatchsimplifies the logic by:
- Eliminating explicit dimension branches
- Using
for_N_make_arrayuniformly for dl, origin, and nbrCell computation- Providing sensible fallbacks (1.0, 0.0) for temporary patches without geometry
This aligns well with the PR's goal of removing per-patch origin handling and is more maintainable.
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (4)
19-19: Type change aligns with AMR-local indexing.The change from
std::array<std::uint32_t, ...>toPoint<int, ...>is appropriate for AMR coordinates, which can be negative in AMR-local index spaces.
34-35: Simplified coordinate computation using layout API.The direct use of
layout->fieldNodeCoordinatesis cleaner than the previous manual computation and aligns with the PR's move toward layout-driven coordinate handling.
61-61: LGTM: Const qualifier placement style change.The change from
const std::map<...>tostd::map<...> constis stylistic (east-const vs west-const) with no functional impact.
107-109: Good defensive practice with bounds checking.The assertion ensures
hqIndexis within bounds before accessing theparamsarray, preventing potential out-of-bounds access.tests/core/data/gridlayout/deriv.py (2)
116-116: GridLayout instantiation updated for per-order configuration.Creating a GridLayout instance per interpolation order with the explicit
interp_orderparameter is appropriate for the test structure.
183-183: fieldCoords API updated to use GridLayout object.The updated signature passing
gl(GridLayout instance) instead of origin aligns with the PR's goal of removing origin-based coordinate computation and using layout-driven coordinates.Also applies to: 198-198
tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (4)
84-85: Good const correctness for time constants.Making
oldTimeandnewTimeconst improves code clarity and prevents accidental modification.
94-116: Excellent refactor to AMR-aware iteration pattern.The refactored initialization:
- Uses unified
AMRGhostBoxForiteration instead of nested dimension-specific loops- Computes position and local indices once per cell
- Provides cleaner, more maintainable code
This aligns well with the PR's goal of using AMR-local indexing throughout.
119-119: Const correctness for zeroTransformation method.Making
zeroTransformation()a const method is appropriate since it doesn't modify object state.
310-343: Clean AMR-aware iteration for interpolation verification.The refactored test:
- Uses structured bindings for
amr_idx/lcl_idxpairs fromdomain_amr_lcl_idx- Computes position from AMR indices and accesses fields via local indices
- Includes const correctness for
interpolateTimeThis is a clean, modern approach consistent with the PR's AMR-local indexing pattern.
tests/core/data/gridlayout/gridlayout_deriv.cpp (5)
11-11: Const correctness for filename parameter.Making the
filenameparameter const is appropriate and consistent with similarreadfunctions elsewhere in the test suite.
34-52: Well-structured refactor to AMR-local indexing for 1D tests.The refactored test demonstrates the new pattern:
- Structured bindings for
amr_idx/lcl_idxpairs- Coordinates computed from AMR indices via
fieldNodeCoordinates- Field access via local indices
- Const correctness for test data
This is clean and maintainable.
88-182: Consistent AMR-local indexing pattern for 2D tests.All 2D derivative tests follow the unified pattern:
- AMR-aware iteration with structured bindings
- Coordinates from
fieldNodeCoordinateswith AMR indices- Field access via local indices
- Correct 2D index flattening using
allocSizeDerivedfor stride calculationThe consistency across tests is excellent.
197-344: Comprehensive 3D test coverage with AMR-local indexing.All six 3D derivative tests (DXBY3D, DYBY3D, DZBY3D, DXEZ3D, DYEZ3D, DZEZ3D) follow the unified AMR-local indexing pattern with correct 3D index flattening. The consistency and coverage are excellent.
77-79: Comment markers are section headers, tests are active.These comment markers (lines 77-79, 186-188) serve as section headers for 2D and 3D test cases. All tests are active and functional, not commented out. This addresses the previous review question about commented tests.
Also applies to: 186-188
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
pyphare/pyphare/core/gridlayout.py(1 hunks)pyphare/pyphare/pharein/__init__.py(0 hunks)pyphare/pyphare/pharein/maxwellian_fluid_model.py(2 hunks)pyphare/pyphare/pharein/simulation.py(1 hunks)src/amr/resources_manager/amr_utils.hpp(2 hunks)src/amr/wrappers/hierarchy.hpp(2 hunks)src/core/data/field/initializers/field_user_initializer.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(10 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp(2 hunks)src/core/data/particles/particle_utilities.hpp(1 hunks)src/core/utilities/box/box.hpp(1 hunks)src/core/utilities/point/point.hpp(1 hunks)src/core/utilities/types.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(1 hunks)tests/amr/data/field/copy_pack/field_data_test_param.hpp(2 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp(1 hunks)tests/amr/data/field/refine/test_tag_strategy.hpp(1 hunks)tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp(4 hunks)tests/amr/messengers/test_messenger_tag_strategy.hpp(1 hunks)tests/core/data/electrons/test_electrons.cpp(4 hunks)tests/core/data/gridlayout/cellCenteredCoordinates.py(6 hunks)tests/core/data/gridlayout/deriv.py(3 hunks)tests/core/data/gridlayout/fieldNodeCoordinates.py(5 hunks)tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp(3 hunks)tests/core/data/gridlayout/gridlayout_deriv.cpp(5 hunks)tests/core/data/gridlayout/gridlayout_deriv.hpp(1 hunks)tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp(5 hunks)tests/core/data/gridlayout/gridlayout_laplacian.cpp(9 hunks)tests/core/data/gridlayout/test_main.cpp(1 hunks)tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp(1 hunks)tests/core/data/particles/test_main.cpp(1 hunks)tests/core/data/particles/test_particle_array_consistency.cpp(1 hunks)tests/core/numerics/ampere/test_main.cpp(10 hunks)tests/core/numerics/faraday/test_main.cpp(17 hunks)tests/core/numerics/ion_updater/test_updater.cpp(1 hunks)tests/core/numerics/ohm/test_main.cpp(10 hunks)tests/initializer/test_initializer.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- pyphare/pyphare/pharein/init.py
- tests/initializer/test_initializer.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/amr/data/field/copy_pack/field_data_test_param.hpp
- pyphare/pyphare/pharein/maxwellian_fluid_model.py
- src/core/data/field/initializers/field_user_initializer.hpp
- tests/core/data/gridlayout/fieldNodeCoordinates.py
- tests/core/data/gridlayout/deriv.py
- tests/core/data/particles/test_main.cpp
- tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp
- pyphare/pyphare/pharein/simulation.py
- tests/core/data/gridlayout/gridlayout_laplacian.cpp
- tests/core/data/gridlayout/test_main.cpp
- tests/amr/messengers/test_messenger_tag_strategy.hpp
- src/core/utilities/types.hpp
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
- tests/core/numerics/ampere/test_main.cpp
🧰 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/particles/particle_utilities.hppsrc/core/utilities/box/box.hpptests/amr/data/field/refine/test_tag_strategy.hppsrc/amr/resources_manager/amr_utils.hppsrc/core/utilities/point/point.hppsrc/diagnostic/diagnostic_model_view.hpptests/core/data/gridlayout/gridlayout_field_centered_coord.hppsrc/amr/wrappers/hierarchy.hpptests/core/data/gridlayout/gridlayout_deriv.hppsrc/core/data/grid/gridlayout.hppsrc/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-09-01T09:41:40.618Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1056
File: tests/core/numerics/interpolator/test_main.cpp:314-318
Timestamp: 2025-09-01T09:41:40.618Z
Learning: The PHARE project uses C++20, which makes the typename keyword optional for dependent types in obvious contexts. The project prefers minimal code without unnecessary keywords, so typename suggestions for dependent types should be avoided when the code compiles successfully in C++20.
Applied to files:
tests/core/data/particles/test_particle_array_consistency.cpptests/core/data/electrons/test_electrons.cpp
📚 Learning: 2025-02-06T10:52:56.461Z
Learnt from: rochSmets
Repo: PHAREHUB/PHARE PR: 921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
Applied to files:
tests/core/data/particles/test_particle_array_consistency.cppsrc/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
📚 Learning: 2025-04-17T14:41:53.643Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 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.
Applied to files:
src/amr/resources_manager/amr_utils.hppsrc/core/utilities/point/point.hpp
🔇 Additional comments (17)
tests/core/data/particles/test_particle_array_consistency.cpp (1)
28-33: LGTM! Variable rename improves clarity.The rename from
bixtoamr_idxbetter reflects the AMR-aware indexing approach and aligns with the PR's objective of replacing origin-based coordinate computations with explicit AMR indexing.src/core/utilities/box/box.hpp (1)
28-28: LGTM! Clean consolidation of type alias.Moving
value_typeto the top of the struct and removing the duplicate declaration improves code organization and follows standard C++ conventions.src/core/data/particles/particle_utilities.hpp (2)
15-18: Good documentation of the AMR index space assumption.The comment clearly states the critical assumption that
iCellmust be in AMR index space. This is essential for understanding the simplified logic below.
24-27: Simplified calculation aligns with PR objectives and improves performance.The direct calculation
(particle.iCell[iDim] + particle.delta[iDim]) * meshSize[iDim]eliminates intermediate origin and index transformations, which should reduce floating-point rounding errors and potential epsilon mismatches—exactly as intended by this PR. The code is also cleaner and more performant.However, verify that all call sites of
positionAsPointensure thatparticle.iCellis correctly populated in AMR index space, as this is now a hard requirement.Run the following script to identify all call sites and verify the assumption:
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
68-68: Correct calculation using AMR-aware indexing.The new calculation derives the physical domain extent directly from AMR box geometry rather than origin offsets, which aligns with the PR's goal of eliminating potential epsilon coordinate mismatches. The formula
(upper + 1) * meshSizecorrectly computes the physical end of the domain.tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
107-110: Good defensive programming with the assertion.The cast to
std::size_tand the subsequent bounds check ensure safe access toparams[hqIndex].tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (3)
84-85: Const correctness improvement.The addition of
constqualifiers tooldTimeandnewTimeproperly indicates these values don't change after initialization.
94-116: Cleaner iteration using AMR-local indexing.The refactored code using
layout.AMRGhostBoxFor(qty)andlayout.AMRToLocal(amr_idx)is more expressive than the previous approach and aligns with the PR's goal of eliminating origin-based coordinate calculations.
324-343: Improved test structure with structured bindings.The use of
for (auto const [amr_idx, lcl_idx] : layout.domain_amr_lcl_idx(destField))provides a clean iteration pattern that's both readable and efficient.tests/core/data/gridlayout/gridlayout_deriv.hpp (1)
28-28: Type alias improves maintainability.Adding
GridLayout_tas a public type alias makes the GridLayout type accessible and simplifies potential future refactoring.tests/core/data/gridlayout/gridlayout_deriv.cpp (2)
77-79: Note: These are section headers, not commented-out code.The lines
// // -----are just comment decorations for the "2D case" section header. The 2D tests below (DXBY2D, DYBY2D, etc.) are all active and functioning. This clarifies the previous review question about commented tests.
41-51: Clean refactor to AMR-local indexing.The new pattern using
ghost_amr_lcl_idxwith structured bindings[amr_idx, lcl_idx]eliminates the need for manual coordinate calculations and aligns with the PR's objective of removing origin-dependent logic.src/amr/wrappers/hierarchy.hpp (1)
256-269: Origin removal is consistent with PR objectives.The domain lower bounds are now initialized to 0, eliminating the per-patch origin concept. This directly addresses the PR's goal of preventing epsilon coordinate mismatches caused by differing per-patch origins. Coordinates are now computed uniformly using AMR index and mesh size.
src/core/data/grid/gridlayout.hpp (4)
454-485: Critical coordinate calculation now origin-independent.Line 481 shows the key change:
position[iDir] = (static_cast<double>(amr_idx[iDir]) + halfCell) * meshSize_[iDir]now computes coordinates directly from AMR index without origin offset. This eliminates the source of epsilon mismatches that motivated this PR.
1519-1549: Well-designed AMRLocalIndexer provides clean iteration API.The
AMRLocalIndexerstruct elegantly maintains synchronized iteration over both AMR and local indices. The structured binding support viaoperator*()returningstd::forward_as_tuple(*amr_it, *lcl_it)enables the cleanfor (auto const [amr_idx, lcl_idx] : ...)pattern seen throughout the codebase.
296-310: Const correctness improvements.The addition of
constqualifiers (auto const icentering,auto const iDir, etc.) throughout these methods improves code clarity and prevents accidental modifications.
890-895: Modern C++20 requires clause usage.The use of
requires(has_physicalQuantity_v<HasQuantity>)is idiomatic C++20 for constraining template overloads. Ensure the codebase consistently targets C++20 or later.
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
Outdated
Show resolved
Hide resolved
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
Outdated
Show resolved
Hide resolved
2 participants
The point of this is to stop using the origin per patch to calculate the coordinate representation of each field used during initialization
In doing so, we switch to using the AMR index, and the mesh size, which is constant across patches, removing potential for epsilon coordinate mismatch due to using the different origins per patch