Conversation
📝 Walkthrough""" WalkthroughThis set of changes modernizes and centralizes the magnetic field refinement and ghost filling logic in the AMR codebase. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HybridHybridMessengerStrategy
participant SAMRAI_RefineAlgorithm
participant MagneticRefinePatchStrategy
participant PatchLevel
User->>HybridHybridMessengerStrategy: regrid/initLevel/postSynchronize
HybridHybridMessengerStrategy->>SAMRAI_RefineAlgorithm: create RefineSchedule for magnetic field
SAMRAI_RefineAlgorithm->>PatchLevel: fillData() (magnetic field components)
PatchLevel->>MagneticRefinePatchStrategy: postprocessRefine (dimension-dependent correction)
MagneticRefinePatchStrategy-->>PatchLevel: update fine face values for consistency
Possibly related PRs
Suggested labels
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: 2
🧹 Nitpick comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
51-61: Consider documenting the purpose of variable fill patterns.The empty variable fill pattern classes inherit from
BoxGeometryVariableFillPatternbut don't override any methods. Consider adding documentation to explain their purpose in the refinement process.Add documentation:
+ // Variable fill pattern for X-component magnetic field refinement class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + // Variable fill pattern for Y-component magnetic field refinement class YVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + // Variable fill pattern for Z-component magnetic field refinement class ZVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
190-197: Consider adding safety checks for schedule access.The code accesses schedules from maps without checking if they exist. While
registerLevelshould be called before these accesses, defensive programming would make the code more robust.Consider adding checks like:
auto it = magGhostsRefineSchedules.find(levelNumber); if (it != magGhostsRefineSchedules.end() && it->second) { it->second->fillData(initDataTime); } else { throw std::runtime_error("Schedule not found for level " + std::to_string(levelNumber)); }Also applies to: 219-220, 257-257, 278-278, 331-331, 650-650, 661-661
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/amr/data/field/field_data.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.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.
src/amr/data/field/field_data.hppsrc/amr/data/field/refine/magnetic_refine_patch_strategy.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (9)
src/amr/data/field/field_data.hpp (1)
49-49: LGTM! Clean type alias addition.The addition of the
gridlayout_typealias properly exposes the template parameter for external use, following standard C++ practices.src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (4)
1-13: LGTM! Header structure and includes are appropriate.The header guards, includes, and namespace usage follow project conventions.
14-38: LGTM! Clean interface implementation.The class structure properly implements the required interface methods. The empty methods are appropriate as this strategy only needs post-refinement processing.
55-66: LGTM! Correct 1D magnetic field interpolation.The linear interpolation for odd indices is appropriate for 1D magnetic field refinement.
68-124: LGTM! Well-implemented 2D magnetic field refinement.The implementation correctly handles staggered grid offsets and includes appropriate cross-component corrections for divergence-free refinement. The comments clearly explain the offset logic.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
159-172: LGTM! Clean component-specific registration.The registration of magnetic field components with their respective variable fill patterns is well-structured and improves clarity over the previous approach.
1053-1058: LGTM! Well-organized schedule storage.The use of
std::mapfor storing schedules by level number provides efficient access and clear organization.
1119-1121: LGTM! Clean instantiation of the refinement strategy.The type alias improves readability and the strategy is properly initialized with the resource manager.
23-23: Well-executed incremental refactoring.The refactoring successfully modernizes the magnetic field refinement to use SAMRAI's
RefineAlgorithmandRefineSchedulewith a custom patch strategy, while maintaining compatibility with existing electric field and particle refinement approaches. This incremental approach reduces risk and maintains system stability.Also applies to: 34-36, 1053-1058, 1120-1121
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
579ef90 to
2b660e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (1)
156-399: 🛠️ Refactor suggestionConsider refactoring the complex 3D expressions for better maintainability.
The 3D refinement formulas are extremely complex with many terms, making them difficult to verify and maintain. Consider breaking these into smaller, well-named intermediate calculations.
Example refactoring approach:
// Extract common sub-expressions auto linear_term_bx = 0.5 * (bx(ix - 1, iy, iz) + bx(ix + 1, iy, iz)); auto second_order_by_term = /* ... extract the 0.125 * (...) term */; auto second_order_bz_term = /* ... extract the 0.125 * (...) term */; auto third_order_by_term = /* ... extract the mesh ratio term */; auto third_order_bz_term = /* ... extract the mesh ratio term */; bx(ix, iy, iz) = linear_term_bx + second_order_by_term + second_order_bz_term + third_order_by_term + third_order_bz_term;This would significantly improve readability and make the implementation easier to verify against the mathematical formulation.
🧹 Nitpick comments (1)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (1)
28-31: Clarify the stencil width comment.The comment mentions "0th order base interpolation" but the method returns 1, not 0. This could be confusing - consider clarifying whether this refers to the interpolation order or the stencil width.
- return SAMRAI::hier::IntVector(dim, 1); // hard-coded 0th order base interpolation + return SAMRAI::hier::IntVector(dim, 1); // stencil width of 1 for refinement operations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.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.
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
🧬 Code Graph Analysis (1)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (1)
src/amr/data/field/field_data.hpp (4)
patch(318-327)patch(318-318)patch(330-339)patch(330-330)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
🔇 Additional comments (7)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (7)
1-7: Header structure looks good.The header guards follow standard conventions and the includes are appropriate for the refinement strategy implementation.
14-21: Class design follows SAMRAI patterns correctly.The template parameters and inheritance structure are appropriate for a custom refinement strategy.
55-66: 1D refinement logic is correct and well-implemented.Simple linear interpolation for odd indices is the appropriate approach for 1D magnetic field refinement.
68-124: 2D refinement logic is well-structured.The lambda functions for offset calculations improve readability, and the implementation correctly applies linear interpolation with cross-component corrections for divergence-free refinement.
126-155: 3D setup section is well-designed.The mesh size extraction and lambda function setup appropriately handle the complexity of 3D refinement calculations. The ijk_factor lambda correctly addresses the indexing differences from the Tóth and Roe formulation.
322-324: Interpolation bug appears to be fixed.The linear interpolation now correctly uses
bz(ix, iy, iz - 1) + bz(ix, iy, iz + 1)instead of the duplicated term mentioned in previous reviews.
402-404: Private member design is appropriate.The resource manager reference storage follows good encapsulation practices.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
2b660e7 to
add1a37
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (2)
52-54: Address static analysis warnings about unused variables.The static analysis tools flag
byandbzas unused, but they are actually used in the 2D and 3D constexpr branches. This appears to be a limitation of the static analysis tool with constexpr if statements.The variables are correctly used in the appropriate dimensional branches, so this can be considered a false positive from the static analysis tools.
68-68: Potential issue with modulo operator on negative numbers.The modulo operator
%with negative numbers in C++ returns a negative result, which could causeix % 2 == 1to fail for negative indices. However, in the SAMRAI AMR context, coordinates are typically non-negative.Consider adding an assertion or comment documenting the assumption that indices are non-negative, or use a more robust oddness check if negative indices are possible.
🧹 Nitpick comments (3)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (3)
8-8: Remove iostream include from header file.Including
<iostream>in a header file can increase compilation time and is generally unnecessary unless the header needs to expose iostream functionality. The only usage appears to be for debug output in line 50.-#include <iostream>
130-400: Consider breaking down the complex 3D implementation.The 3D refinement formulas are extremely complex with repetitive patterns. This makes the code difficult to read, maintain, and verify for correctness.
Consider extracting helper functions for:
- The common interpolation patterns
- Cross-component correction terms
- Third-order correction terms
This would improve readability and make it easier to verify the mathematical correctness against the Toth and Roe (2002) reference.
Example refactor approach:
private: template<typename FieldT> auto computeLinearInterpolation(FieldT& field, int i, int j, int k, int dir) const; template<typename FieldT> auto computeCrossComponentCorrection(FieldT& field1, FieldT& field2, /* params */) const; template<typename FieldT> auto computeThirdOrderCorrection(FieldT& field, /* params */) const;
30-33: Consider making stencil width configurable.The hard-coded stencil width of 1 is documented as "0th order base interpolation," but the actual refinement uses higher-order formulas. Consider whether this should be configurable or if the comment needs clarification.
The comment might be misleading since the actual implementation uses higher-order corrections. Consider clarifying or making the stencil width match the actual computational requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.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.
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (2)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (2)
78-82: Well-designed lambda functions for offset calculations.The
p_plus,p_minus,d_plus, andd_minuslambda functions provide a clean abstraction for the offset calculations used in the refinement formulas. This improves code readability despite the complex underlying mathematics.
154-155: Good documentation of indexing differences.The comment explaining the difference from Toth and Roe's centered indexing and the
ijk_factorlambda provide valuable context for understanding the implementation differences from the reference paper.
UCaromel
force-pushed
the
refine-patch-strategy
branch
2 times, most recently
from
eddecdf to
795c415
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
200-200: Commented-out code should be removed.Multiple lines contain commented-out code that should either be removed or converted to proper documentation if the information is still relevant.
This matches the existing review comments from github-advanced-security[bot] about commented-out code.
Also applies to: 204-205, 355-355, 379-379, 428-428, 747-747, 758-758
🧹 Nitpick comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
52-62: Add documentation for the empty fill pattern classes.These empty classes inherit from
BoxGeometryVariableFillPatternbut lack any documentation explaining their purpose, usage, or why they are component-specific (X, Y, Z). Consider adding class-level documentation explaining their role in the magnetic field refinement strategy.+ /** + * @brief Fill pattern for X-component magnetic field refinement. + * Used with SAMRAI's RefineAlgorithm for component-specific refinement behavior. + */ class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + + /** + * @brief Fill pattern for Y-component magnetic field refinement. + * Used with SAMRAI's RefineAlgorithm for component-specific refinement behavior. + */ class YVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + + /** + * @brief Fill pattern for Z-component magnetic field refinement. + * Used with SAMRAI's RefineAlgorithm for component-specific refinement behavior. + */ class ZVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
294-353: Complex mathematical formulas could benefit from better documentation.The 2D divergence-free refinement formulas implement sophisticated mathematical operations but lack sufficient documentation explaining the algorithm, its derivation, and the meaning of variables like
p_plus,p_minus,d_plus,d_minus.Add comprehensive documentation explaining:
- The mathematical basis for the divergence-free refinement
- The meaning and purpose of the offset calculations
- References to the Tóth and Roe (2002) paper mentioned in the PR objectives
- Examples of how the formulas maintain divergence-free properties
+ // Divergence-free refinement formulas based on Tóth and Roe (2002) + // These formulas ensure that newly refined magnetic field values maintain + // the divergence-free constraint: ∇ · B = 0 + // + // p_plus/p_minus: Primal grid point indexing with offset + // d_plus/d_minus: Dual grid point indexing with offset
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(16 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/amr/data/field/field_variable_fill_pattern.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
150-174: Well-structured magnetic field refinement registration.The new approach using SAMRAI's
RefineAlgorithmwith component-specific fill patterns is well-organized and follows SAMRAI conventions. The separation of face-centered (Balgo) and node-centered (BalgoNode) algorithms is appropriate for magnetic field refinement.
191-222: Improved refinement schedule management.The new schedule-based approach using
RefineAlgorithm::createSchedule()provides better control over refinement operations compared to the previousRefinerPoolapproach. The proper separation of different schedule types (shared nodes, patch ghosts, level ghosts, initialization) is well-designed.
1150-1156: Appropriate use of STL containers for schedule management.The use of
std::mapfor storing refinement schedules indexed by level number is appropriate and provides efficient lookup. The shared pointer usage for SAMRAI schedule objects follows proper RAII principles.
1216-1218: Well-templated magnetic refinement patch strategy.The
MagneticRefinePatchStrategyis properly templated with the resource manager and field data types, and correctly initialized with the resource manager reference. This follows good C++ design patterns.
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
795c415 to
204e4df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
200-200: 🛠️ Refactor suggestionRemove commented-out code.
Multiple lines contain commented-out code that should be removed for cleaner codebase maintenance. This matches existing static analysis findings.
Consider removing all commented-out code and using version control history if these lines need to be referenced later.
Also applies to: 204-205, 371-371, 395-395, 444-444, 763-763, 774-774
256-369: 🛠️ Refactor suggestionExtract complex 2D refinement logic into separate methods.
The magnetic field refinement logic in the
regridmethod is highly complex with nested lambdas, mathematical calculations, and dimension-specific branches. This makes the method difficult to read, test, and maintain.Consider extracting the refinement logic into separate private methods:
+ private: + void setMagneticFieldToNaN(HybridModel& hybridModel, SAMRAI::hier::PatchLevel& level); + void performDivergenceFreeRefinement2D(HybridModel& hybridModel, SAMRAI::hier::PatchLevel& level); + void postprocessBxComponent1D(VecFieldT& B, GridLayoutT const& layout); + void postprocessBxComponent2D(VecFieldT& B, GridLayoutT const& layout); + void postprocessByComponent2D(VecFieldT& B, GridLayoutT const& layout);This would:
- Improve readability by separating concerns
- Enable easier unit testing of individual refinement formulas
- Reduce the complexity of the main
regridmethod- Make the mathematical formulas more self-documenting
282-286: 🛠️ Refactor suggestionAddress unused variables flagged by static analysis.
Static analysis correctly identifies unused variables
layout,bx, andbyin this scope. These variables are declared but not used in the subsequent lambda functions.Remove the unused variable declarations or use them if they were intended for the postprocessing logic:
auto const& layout = layoutFromPatch<GridLayoutT>(*patch); auto _ = resourcesManager_->setOnPatch(*patch, hybridModel.state.electromag.B); auto& B = hybridModel.state.electromag.B; - auto& bx = B(core::Component::X); - auto& by = B(core::Component::Y);
🧹 Nitpick comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
52-62: Consider adding documentation for empty fill pattern classes.The empty
XVariableFillPattern,YVariableFillPattern, andZVariableFillPatternclasses serve as markers for different magnetic field components, but their purpose isn't immediately clear from the code.Consider adding brief documentation comments:
+ /** @brief Fill pattern for X-component of magnetic field */ class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + /** @brief Fill pattern for Y-component of magnetic field */ class YVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + /** @brief Fill pattern for Z-component of magnetic field */ class ZVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(16 hunks)tests/functional/harris/harris_2d.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/amr/data/field/field_variable_fill_pattern.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🪛 Pylint (3.3.7)
tests/functional/harris/harris_2d.py
[error] 4-4: Unable to import 'numpy'
(E0401)
[error] 5-5: Unable to import 'pyphare.pharein'
(E0401)
[error] 6-6: Unable to import 'pyphare.cpp'
(E0401)
[error] 7-7: Unable to import 'pyphare.simulator.simulator'
(E0401)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (5)
tests/functional/harris/harris_2d.py (1)
4-4: LGTM! Good import organization.The consolidation of imports from the same module and the numpy import reorganization follows Python best practices for import styling.
Also applies to: 7-7
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'numpy'
(E0401)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
150-174: LGTM! Well-structured magnetic field registration.The registration of magnetic field components with their respective variable fill patterns and refinement operators is well-organized and follows a clear pattern for both
BalgoandBalgoNodealgorithms.
1166-1171: Well-designed refinement schedule management.The new member variables for managing SAMRAI refinement algorithms and schedules provide a clean separation of concerns for different types of magnetic field refinement operations (initialization, ghosts, patch ghosts, shared nodes).
1232-1234: Good integration of magnetic refinement patch strategy.The integration of
MagneticRefinePatchStrategywith proper template parameters and resource manager dependency injection follows good design principles.
312-362:Details
❓ Verification inconclusive
Verify the mathematical correctness of 2D divergence-free refinement formulas.
The 2D refinement formulas implement complex stencil operations for
postprocessBxandpostprocessBywith offset calculations and cross-component dependencies. These formulas appear to implement Tóth-Roe inspired divergence-free refinement.The mathematical formulas are intricate and critical for maintaining magnetic field divergence properties. Please verify:
- The offset calculations (
xoffset,yoffset) align with the intended stencil patterns- The sign patterns in the cross-derivative terms are correct
- The refinement formulas preserve the divergence-free property
🏁 Script executed:
#!/bin/bash # Description: Search for test cases or documentation that verify these refinement formulas # Expected: Find unit tests or references that validate the mathematical correctness # Search for tests related to magnetic field refinement rg -A 10 -B 5 "divergence.*free|divB.*test|magnetic.*refine.*test" --type cpp # Look for references to Toth-Roe or similar divergence-free methods rg -A 5 -B 5 "Toth|Roe|divergence.*free.*refine" --type cpp --type hpp # Search for mathematical validation or documentation rg -A 5 "postprocess.*B[xy]|refinement.*formula" --type cpp --type hppLength of output: 281
Verify divergence-free 2D refinement formulas
I didn’t find any unit tests or documentation in the codebase that exercise or validate these stencils, so please manually confirm the following for the block in
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(lines 312–362):
- The
xoffset/yoffsetcalculations in bothpostprocessBxandpostprocessBymatch the intended stencil patterns for primal/dual indexing.- The sign pattern in the cross-component correction terms (
+ by(...) - by(...) - by(...) + by(...), and similarly forbx) is correct.- These refinement formulas indeed preserve ∇·B=0 on the refined grid.
- If available, link or add unit tests or a reference (e.g. Tóth–Roe) that mathematically validates this implementation.
UCaromel
force-pushed
the
refine-patch-strategy
branch
2 times, most recently
from
6f1134e to
dea0d33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
256-370: Extract complex refinement logic into separate methods.The regrid method contains highly complex 2D divergence-free refinement formulas with nested lambdas and mathematical calculations that significantly impact readability and maintainability.
This matches the previous review suggestion to extract the refinement logic into separate private methods for better organization.
282-286: Address unused variables flagged by static analysis.Static analysis has identified unused local variables:
layout,bx, andby. These should either be used or removed to clean up the code.The static analysis warnings indicate these variables are declared but not used in their respective scopes.
200-200: Remove commented-out code or provide justification.Multiple lines contain commented-out code from the previous RefinerPool implementation. This should either be removed for code cleanliness or accompanied by TODO comments explaining the transition plan.
The static analysis has flagged these as commented-out code that should be addressed.
Also applies to: 204-205, 371-371, 395-395, 444-444, 763-763, 774-774
🧹 Nitpick comments (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
52-62: Document the purpose of empty variable fill pattern classes.The three new classes
XVariableFillPattern,YVariableFillPattern, andZVariableFillPatternare empty subclasses ofBoxGeometryVariableFillPattern. While this might be intentional for component-specific behavior, it should be documented.Consider adding brief documentation comments explaining their purpose:
+ // Variable fill pattern for X-component of magnetic field class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + // Variable fill pattern for Y-component of magnetic field class YVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { }; + // Variable fill pattern for Z-component of magnetic field class ZVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
288-293: Consider extracting helper lambdas to improve readability.The mathematical helper lambdas
p_plus,p_minus,d_plus,d_minusare defined inline but could be extracted as private methods or constants for better reusability and testing.Consider extracting these as private static constexpr functions:
+ private: + static constexpr auto p_plus = [](auto const i, auto const offset) { return i + 2 - offset; }; + static constexpr auto p_minus = [](auto const i, auto const offset) { return i - offset; }; + static constexpr auto d_plus = [](auto const i, auto const offset) { return i + 1 - offset; }; + static constexpr auto d_minus = [](auto const i, auto const offset) { return i - offset; };
1166-1172: Follow consistent naming conventions for member variables.The new SAMRAI refinement algorithm and schedule members use inconsistent naming compared to existing member variables (which use trailing underscores).
Apply consistent naming convention:
- SAMRAI::xfer::RefineAlgorithm Balgo; - SAMRAI::xfer::RefineAlgorithm BalgoNode; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magInitRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magGhostsRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magPatchGhostsRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magSharedNodeRefineSchedules; + SAMRAI::xfer::RefineAlgorithm Balgo_; + SAMRAI::xfer::RefineAlgorithm BalgoNode_; + std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magInitRefineSchedules_; + std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magGhostsRefineSchedules_; + std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magPatchGhostsRefineSchedules_; + std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magSharedNodeRefineSchedules_;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(16 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)tests/functional/harris/harris_2d.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/amr/data/field/field_variable_fill_pattern.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/core/data/grid/gridlayout.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧬 Code Graph Analysis (1)
src/core/data/grid/gridlayout.hpp (1)
src/simulator/simulator.hpp (1)
interp_order(59-59)
🪛 Pylint (3.3.7)
tests/functional/harris/harris_2d.py
[error] 4-4: Unable to import 'numpy'
(E0401)
[error] 5-5: Unable to import 'pyphare.pharein'
(E0401)
[error] 6-6: Unable to import 'pyphare.cpp'
(E0401)
[error] 7-7: Unable to import 'pyphare.simulator.simulator'
(E0401)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
tests/functional/harris/harris_2d.py (1)
4-4: LGTM: Import reorganizationThe consolidation of imports and minor reordering improve code organization without affecting functionality.
Also applies to: 7-7
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'numpy'
(E0401)
src/core/data/grid/gridlayout.hpp (1)
1327-1334: LGTM: Ensure even dual ghost count for refinement operationsThe modification correctly ensures that the number of dual ghost nodes is always even by conditionally adding 1 when the computed value is odd. This supports the magnetic field refinement infrastructure by maintaining consistent ghost node counts for interpolation operations.
The constexpr nature and return type are preserved, and the logic is straightforward and correct.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
1232-1234: Approve the magnetic refine patch strategy integration.The addition of
MagneticRefinePatchStrategymember properly integrates the new refinement approach with the existing resource manager pattern.
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
dea0d33 to
30279d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyphare/pyphare/core/gridlayout.py (1)
232-233: Fix spacing and consider naming convention.The logic for ensuring even ghost cell count is correct and aligns well with the divergence-free refinement implementation. However, there are minor style improvements needed:
- nGhosts = int((interpOrder + 1) / 2) + self.particleGhostNbr(interpOrder) - return nGhosts if nGhosts% 2 == 0 else nGhosts + 1 + n_ghosts = int((interpOrder + 1) / 2) + self.particleGhostNbr(interpOrder) + return n_ghosts if n_ghosts % 2 == 0 else n_ghosts + 1This addresses:
- Missing space around the modulo operator (
%)- Snake_case naming convention to follow Python standards and resolve the pylint warning
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 232-232: Variable name "nGhosts" doesn't conform to snake_case naming style
(C0103)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyphare/pyphare/core/gridlayout.py(1 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(0 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(16 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)tests/functional/harris/harris_2d.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/field/refine/magnetic_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/data/grid/gridlayout.hpp
- src/amr/data/field/field_variable_fill_pattern.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/functional/harris/harris_2d.py (1)
pyphare/pyphare/simulator/simulator.py (2)
Simulator(68-309)startMPI(39-43)
🪛 Pylint (3.3.7)
pyphare/pyphare/core/gridlayout.py
[convention] 232-232: Variable name "nGhosts" doesn't conform to snake_case naming style
(C0103)
tests/functional/harris/harris_2d.py
[error] 4-4: Unable to import 'numpy'
(E0401)
[error] 5-5: Unable to import 'pyphare.pharein'
(E0401)
[error] 6-6: Unable to import 'pyphare.cpp'
(E0401)
[error] 7-7: Unable to import 'pyphare.simulator.simulator'
(E0401)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (2)
tests/functional/harris/harris_2d.py (2)
4-4: LGTM: Good import organization.Moving the numpy import maintains proper import ordering conventions.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'numpy'
(E0401)
7-7: LGTM: Improved import consolidation.Consolidating the imports from the same module into a single line follows Python best practices and improves code readability.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 7-7: Unable to import 'pyphare.simulator.simulator'
(E0401)
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
89cf2af to
fa02433
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (7)
200-200: Remove commented-out code.This commented-out code is flagged by static analysis and should be removed to improve code cleanliness.
204-205: Remove commented-out code.This commented-out code is flagged by static analysis and should be removed to improve code cleanliness.
223-223: Remove commented-out code.This commented-out code should be removed as part of the migration to the new refinement approach.
254-254: Remove commented-out code.This commented-out code is flagged by static analysis and should be removed to improve code cleanliness.
646-646: Remove commented-out code.This commented-out code is flagged by static analysis and should be removed to improve code cleanliness.
657-657: Remove commented-out code.This commented-out code is flagged by static analysis and should be removed to improve code cleanliness.
792-1205: Consider extracting the complex refinement logic into separate methods.The
magneticRegriding_method is extremely complex with 400+ lines of nested mathematical calculations. This makes it difficult to read, test, and maintain.Consider extracting the dimension-specific refinement logic into separate private methods:
setMagneticFieldToNaNperformDivergenceFreeRefinement1DperformDivergenceFreeRefinement2DperformDivergenceFreeRefinement3DThis would significantly improve readability and maintainability.
🧹 Nitpick comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
52-62: Consider documenting the purpose of empty fill pattern classes.The empty
XVariableFillPattern,YVariableFillPattern, andZVariableFillPatternclasses serve as type markers for different magnetic field components. Adding brief documentation would clarify their intended use in the refinement process.+ // Empty fill pattern classes used as type markers for magnetic field components + // These inherit default behavior from BoxGeometryVariableFillPattern class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(17 hunks)
🧰 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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧬 Code Graph Analysis (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (8)
src/amr/level_initializer/hybrid_level_initializer.hpp (4)
levelNumber(39-39)levelNumber(39-39)hierarchy(46-164)hierarchy(46-49)src/amr/messengers/refiner_pool.hpp (2)
levelNumber(107-113)levelNumber(107-107)src/amr/messengers/refiner.hpp (2)
levelNumber(152-159)levelNumber(152-152)src/amr/messengers/hybrid_messenger_strategy.hpp (5)
level(116-116)hierarchy(52-53)hierarchy(56-59)E(70-70)patch(33-33)src/amr/messengers/hybrid_messenger.hpp (13)
level(191-191)level(191-191)hierarchy(112-116)hierarchy(112-113)hierarchy(123-129)E(273-276)E(273-273)E(333-333)E(333-333)patch(84-87)patch(84-84)B(332-332)B(332-332)src/amr/resources_manager/amr_utils.hpp (2)
layoutFromPatch(135-187)layoutFromPatch(135-135)src/core/data/field/field.hpp (4)
args(77-83)args(77-77)args(85-88)args(85-85)src/amr/resources_manager/resources_manager.hpp (1)
dimension(85-290)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- 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)
🔇 Additional comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
150-174: LGTM! Well-structured magnetic field registration.The registration of magnetic field components with distinct fill patterns and both regular and node-based refinement algorithms follows SAMRAI conventions and supports the divergence-free refinement strategy.
1244-1312: LGTM! Well-structured member variables for the new refinement approach.The addition of SAMRAI refinement algorithms, schedule maps, and the magnetic refinement patch strategy follows good design patterns and naming conventions.
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
6c855c9 to
691d191
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (11)
200-200: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
204-205: Remove commented-out code.These lines contain commented-out code that should be removed to keep the codebase clean.
254-254: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
278-278: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
327-327: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
646-646: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
657-657: Remove commented-out code.This line contains commented-out code that should be removed to keep the codebase clean.
931-931:⚠️ Potential issueFix copy-paste bug in 3D z-index assignment.
The z-index
izis incorrectly assignedidx[dirY]instead ofidx[dirZ].- auto iz = idx[dirY]; + auto iz = idx[dirZ];
1020-1020:⚠️ Potential issueFix copy-paste bug in 3D z-index assignment.
The z-index
izis incorrectly assignedidx[dirY]instead ofidx[dirZ].- auto iz = idx[dirY]; + auto iz = idx[dirZ];
1109-1109:⚠️ Potential issueFix copy-paste bug in 3D z-index assignment.
The z-index
izis incorrectly assignedidx[dirY]instead ofidx[dirZ].- auto iz = idx[dirZ]; + auto iz = idx[dirZ];
793-1205: 🛠️ Refactor suggestionExtract complex refinement logic into separate methods.
The
magneticRegriding_method is extremely long and complex, making it difficult to read, test, and maintain. The nested lambda functions and dimension-specific mathematical calculations should be extracted into separate private methods.Consider extracting methods like:
setMagneticFieldToNaN()performDivergenceFreeRefinement1D()performDivergenceFreeRefinement2D()performDivergenceFreeRefinement3D()- Individual postprocessing methods for each component and dimension
This would significantly improve readability, testability, and maintainability.
🧹 Nitpick comments (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
52-62: Consider removing unnecessary empty fill pattern classes.The three fill pattern classes (
XVariableFillPattern,YVariableFillPattern,ZVariableFillPattern) are empty and don't add any functionality beyond their base class. If these are just placeholders for future functionality, consider using the base class directly until specific behavior is needed.- class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern - { - }; - - class YVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern - { - }; - - class ZVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern - { - };And use
std::make_shared<SAMRAI::xfer::BoxGeometryVariableFillPattern>()directly in the registration code.
672-680: Remove commented-out code blocks.These commented-out code blocks should be removed to maintain code cleanliness.
718-719: Remove commented-out code blocks.These commented-out code blocks should be removed to maintain code cleanliness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(17 hunks)
🧰 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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
150-174: LGTM: Proper SAMRAI refinement registration.The magnetic field component registration with custom fill patterns follows SAMRAI conventions correctly. The separation of
BalgoandBalgoNodealgorithms for different refinement types is appropriate.
191-221: LGTM: Well-structured refinement schedule creation.The creation and storage of refinement schedules in maps keyed by level number is a clean approach. The different schedule types (shared nodes, patch ghosts, ghosts, init) are properly differentiated and created with appropriate parameters.
1244-1250: LGTM: Well-structured member variables for refinement.The new SAMRAI refinement algorithm objects and schedule maps are properly declared and follow good naming conventions. The use of maps keyed by level number is an appropriate pattern for per-level schedule storage.
1310-1312: LGTM: Appropriate magnetic refinement patch strategy member.The
MagneticRefinePatchStrategymember variable is properly declared with template parameters and resource manager dependency.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (4)
62-63: Remove unused variables to clean up the code.Variables
byandbzare declared but never used in this scope, as they're only needed within the dimension-specific refinement blocks below.Consider moving these variable declarations closer to their usage or removing them entirely if not needed:
- auto& by = FieldDataT::getField(fine, *by_id); - auto& bz = FieldDataT::getField(fine, *bz_id);
186-186:⚠️ Potential issueFix the oddness check to handle negative numbers correctly.
The modulo operation
ix % 2 == 1fails for negative numbers. Use a proper oddness check that works for all integers.Apply this fix:
- if (ix % 2 == 1) + if ((ix & 1) == 1 || (ix < 0 && (-ix & 1) == 1))Or use a helper function for clarity:
+ auto isOdd = [](int i) { return (i & 1) == 1; }; - if (ix % 2 == 1) + if (isOdd(ix))
218-218:⚠️ Potential issueFix the oddness check to handle negative numbers correctly.
Same issue as line 186 - the modulo operation fails for negative numbers.
Apply the same fix as suggested for line 186:
- if (ix % 2 == 1) + if (isOdd(ix))
247-247:⚠️ Potential issueFix the oddness check to handle negative numbers correctly.
Same issue as previous oddness checks - needs to handle negative numbers properly.
Apply the same fix:
- if (iy % 2 == 1) + if (isOdd(iy))
🧹 Nitpick comments (3)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (3)
280-284: Improve code documentation for complex mathematical formulation.The comment about Toth and Roe (2002) indexing differences is helpful, but the
ijk_factorlambda could benefit from clearer documentation about its purpose in the divergence-free refinement formula.Consider expanding the comment:
// Toth and Roe (2002) use a formulation we the indexing is centered // on the coarse cell. Since this is not our case, we need to have a // different offset for indexing and applying the +-1 factor to the // third order terms. That's the job of the ijk_factor lambda + // ijk_factor applies the sign correction needed for third-order terms + // based on whether we're at offset 0 (negative sign) or offset 1 (positive sign) auto ijk_factor = [](auto const offset) { return offset == 0 ? -1 : 1; };
304-375: Consider refactoring the complex 3D Bx refinement formula for better maintainability.The 3D refinement formula for Bx is very long and complex, making it difficult to verify correctness and maintain. Consider breaking it into smaller, named components or functions.
For example, you could extract the different terms:
auto linearTerm = 0.5 * (bx(ix - 1, iy, iz) + bx(ix + 1, iy, iz)); auto secondOrderBy = 0.125 * computeSecondOrderByTerm(ix, iy, iz, xoffset, yoffset, zoffset); auto secondOrderBz = 0.125 * computeSecondOrderBzTerm(ix, iy, iz, xoffset, yoffset, zoffset); auto thirdOrderBy = computeThirdOrderByTerm(ix, iy, iz, xoffset, yoffset, zoffset); auto thirdOrderBz = computeThirdOrderBzTerm(ix, iy, iz, xoffset, yoffset, zoffset); bx(ix, iy, iz) = linearTerm + secondOrderBy + secondOrderBz + thirdOrderBy + thirdOrderBz;This would improve readability and make the implementation easier to verify against the Toth and Roe (2002) paper.
26-26: Consider adding static assertion for supported dimensions.The template assumes certain dimensional constraints but doesn't enforce them at compile time.
static constexpr std::size_t dimension = FieldDataT::dimension; + static_assert(dimension >= 1 && dimension <= 3, + "MagneticRefinePatchStrategy only supports 1D, 2D, and 3D");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/amr/data/field/field_geometry.hpp(2 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(17 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/data/field/field_geometry.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.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.
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
🧬 Code Graph Analysis (1)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (3)
src/amr/data/field/field_data.hpp (4)
patch(318-327)patch(318-318)patch(330-339)patch(330-330)pyphare/pyphare/pharesee/geometry.py (1)
toFieldBox(11-28)src/amr/data/field/field_geometry.hpp (4)
box(156-259)box(156-157)box(282-291)box(282-282)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (5)
52-62: Consider documenting the purpose of empty fill pattern classes.The three empty fill pattern classes (
XVariableFillPattern,YVariableFillPattern,ZVariableFillPattern) inherit fromSAMRAI::xfer::BoxGeometryVariableFillPatternbut contain no implementation. While this may be intentional for distinguishing variable types in the magnetic field refinement system, the purpose should be documented.Add brief documentation explaining why these classes are empty and their role in the magnetic refinement strategy:
+ // Empty fill pattern classes used to distinguish magnetic field components + // in the refinement algorithm. Each component (X, Y, Z) uses a separate + // fill pattern for proper handling during divergence-free refinement. class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
167-176: Improve code readability with consistent variable naming.The variable names
BalgoandBalgoNodeare not descriptive of their purpose. Consider using more meaningful names that indicate they are magnetic field refinement algorithms.Apply this diff to improve variable naming:
- Balgo.registerRefine(*bx_id, *bx_id, *bx_id, BfieldRefineOp_, xVariableFillPattern); - Balgo.registerRefine(*by_id, *by_id, *by_id, BfieldRefineOp_, yVariableFillPattern); - Balgo.registerRefine(*bz_id, *bz_id, *bz_id, BfieldRefineOp_, zVariableFillPattern); + magneticRefineAlgo.registerRefine(*bx_id, *bx_id, *bx_id, BfieldRefineOp_, xVariableFillPattern); + magneticRefineAlgo.registerRefine(*by_id, *by_id, *by_id, BfieldRefineOp_, yVariableFillPattern); + magneticRefineAlgo.registerRefine(*bz_id, *bz_id, *bz_id, BfieldRefineOp_, zVariableFillPattern); - BalgoNode.registerRefine(*bx_id, *bx_id, *bx_id, BfieldNodeRefineOp_, + magneticNodeRefineAlgo.registerRefine(*bx_id, *bx_id, *bx_id, BfieldNodeRefineOp_, xVariableFillPattern);
252-265: Consider extracting magnetic regrid logic into a separate method.This code block creates a separate
MagneticRefinePatchStrategyinstance specifically for regridding, registers IDs, and creates/executes a schedule. This logic could be extracted into a private method to improve readability and reusability.Extract this logic into a private method:
+ private: + void regridMagneticFields(SAMRAI::hier::PatchLevel& level, + std::shared_ptr<SAMRAI::hier::PatchLevel> const& oldLevel, + HybridModel& hybridModel, + std::shared_ptr<SAMRAI::hier::PatchHierarchy> const& hierarchy, + double const initDataTime) { + MagneticRefinePatchStrategy<ResourcesManagerT, FieldDataT> magneticRegridPatchStrategy{ + *resourcesManager_, make_optional(oldLevel)}; + + core::VecFieldNames BNames{hybridModel.state.electromag.B}; + magneticRegridPatchStrategy.registerIDs(resourcesManager_->getID(BNames.xName), + resourcesManager_->getID(BNames.yName), + resourcesManager_->getID(BNames.zName)); + + auto magSchedule = Balgo.createSchedule(level, oldLevel, + level->getNextCoarserHierarchyLevelNumber(), + hierarchy, &magneticRegridPatchStrategy); + magSchedule->fillData(initDataTime); + }Then replace the current code block with:
- MagneticRefinePatchStrategy<ResourcesManagerT, FieldDataT> magneticRegridPatchStrategy{ - *resourcesManager_, make_optional(oldLevel)}; - - core::VecFieldNames BNames{hybridModel.state.electromag.B}; - - magneticRegridPatchStrategy.registerIDs(resourcesManager_->getID(BNames.xName), - resourcesManager_->getID(BNames.yName), - resourcesManager_->getID(BNames.zName)); - - auto magSchedule - = Balgo.createSchedule(level, oldLevel, level->getNextCoarserHierarchyLevelNumber(), - hierarchy, &magneticRegridPatchStrategy); - magSchedule->fillData(initDataTime); + regridMagneticFields(*level, oldLevel, hybridModel, hierarchy, initDataTime);
814-820: Improve member variable naming consistency.The member variables
BalgoandBalgoNodeuse inconsistent naming compared to other members in the class that follow a descriptive naming pattern with underscores.Apply consistent naming conventions:
- SAMRAI::xfer::RefineAlgorithm Balgo; - SAMRAI::xfer::RefineAlgorithm BalgoNode; + SAMRAI::xfer::RefineAlgorithm magneticRefineAlgo_; + SAMRAI::xfer::RefineAlgorithm magneticNodeRefineAlgo_;This change would require updating all references to these variables throughout the file.
816-819: Consider using consistent container type for schedule maps.The schedule maps use
std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>>while other member containers in the class use different patterns. Consider usingstd::unordered_mapfor better performance if order is not important.If insertion order is not critical for these schedules, consider using unordered_map:
- std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magInitRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magGhostsRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magPatchGhostsRefineSchedules; - std::map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magSharedNodeRefineSchedules; + std::unordered_map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magInitRefineSchedules_; + std::unordered_map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magGhostsRefineSchedules_; + std::unordered_map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magPatchGhostsRefineSchedules_; + std::unordered_map<int, std::shared_ptr<SAMRAI::xfer::RefineSchedule>> magSharedNodeRefineSchedules_;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(14 hunks)src/amr/messengers/refiner.hpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/messengers/refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/amr/data/field/refine/magnetic_field_refiner.hpp (1)
23-30: Documentation successfully addresses previous feedback.The class-level documentation clearly explains the refinement operator's purpose and working assumptions, directly addressing the previous review comment. The explanation of zero-order interpolation and the role of
MagneticRefinePatchStrategyfor inner fine faces provides good context.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
161-177:⚠️ Potential issueCritical: Add error checking for resource ID retrieval.
The code retrieves magnetic field component IDs and immediately dereferences them without checking validity. This could cause undefined behavior if a component name isn't registered.
Apply this diff to add proper error checking:
auto bx_id = resourcesManager_->getID(hybridInfo->modelMagnetic.xName); auto by_id = resourcesManager_->getID(hybridInfo->modelMagnetic.yName); auto bz_id = resourcesManager_->getID(hybridInfo->modelMagnetic.zName); +if (!bx_id || !by_id || !bz_id) { + throw std::runtime_error("Failed to retrieve magnetic field component ID(s): " + + hybridInfo->modelMagnetic.xName + ", " + + hybridInfo->modelMagnetic.yName + ", " + + hybridInfo->modelMagnetic.zName); +} magneticRefinePatchStrategy_.registerIDs(bx_id, by_id, bz_id);
194-202: 🛠️ Refactor suggestionAdd error handling for schedule creation.
Schedule creation can potentially fail, but the code doesn't verify success before storing schedules in maps.
- magSharedNodeRefineSchedules[levelNumber] - = BalgoNode.createSchedule(level, &magneticRefinePatchStrategy_); + auto sharedNodeSchedule = BalgoNode.createSchedule(level, &magneticRefinePatchStrategy_); + if (!sharedNodeSchedule) { + throw std::runtime_error("Failed to create magnetic shared node refine schedule for level " + std::to_string(levelNumber)); + } + magSharedNodeRefineSchedules[levelNumber] = sharedNodeSchedule;Apply similar error checking for all schedule creation calls.
Also applies to: 221-222
771-1183: 🛠️ Refactor suggestionExtract complex mathematical refinement logic into separate methods.
The
magneticRegriding_method is extremely long and contains complex nested lambda functions with mathematical calculations. This makes it difficult to read, test, and maintain.Consider extracting the refinement logic into separate private methods:
private: void setMagneticFieldToNaN(HybridModel& hybridModel, SAMRAI::hier::PatchLevel& level); void performDivergenceFreeRefinement1D(VecFieldT& B, GridLayoutT const& layout); void performDivergenceFreeRefinement2D(VecFieldT& B, GridLayoutT const& layout); void performDivergenceFreeRefinement3D(VecFieldT& B, GridLayoutT const& layout);This would significantly improve readability and enable easier unit testing of the mathematical formulas.
🧹 Nitpick comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
52-62: Document the purpose of empty fill pattern classes.These empty subclasses of
SAMRAI::xfer::BoxGeometryVariableFillPatternserve as type markers for different magnetic field components, but their purpose isn't clear from the code alone.+ // Custom fill patterns for magnetic field components to enable + // component-specific refinement behavior in SAMRAI schedules class XVariableFillPattern : public SAMRAI::xfer::BoxGeometryVariableFillPattern { };
816-821: Consider extracting utility functions for improved readability.The lambda functions
p_plus,p_minus,d_plus,d_minusimplement mathematical offset calculations that are used throughout the 3D refinement logic.Consider extracting these as static member functions or free functions in a utility namespace to improve code organization and reusability:
namespace refinement_utils { constexpr auto p_plus = [](auto const i, auto const offset) { return i + 2 - offset; }; constexpr auto p_minus = [](auto const i, auto const offset) { return i - offset; }; constexpr auto d_plus = [](auto const i, auto const offset) { return i + 1 - offset; }; constexpr auto d_minus = [](auto const i, auto const offset) { return i - offset; }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/amr/data/field/refine/magnetic_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/amr/data/field/refine/magnetic_field_refiner.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧬 Code Graph Analysis (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (4)
bx_id(33-41)bx_id(33-33)patch(43-46)patch(43-44)src/amr/data/field/field_data.hpp (4)
patch(318-327)patch(318-318)patch(330-339)patch(330-330)src/amr/resources_manager/amr_utils.hpp (2)
layoutFromPatch(135-187)layoutFromPatch(135-135)src/amr/resources_manager/resources_manager.hpp (1)
dimension(85-290)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
9-9:Details
✅ Verification successful
Verify usage of included header.
The
"core/utilities/index/index.hpp"include was added but it's unclear if it's actually used in this file.
🏁 Script executed:
#!/bin/bash # Search for usage of index utilities in the file rg -n "index::|MeshIndex|dirX|dirY|dirZ" src/amr/messengers/hybrid_hybrid_messenger_strategy.hppLength of output: 1546
No action needed:
index.hppis used
The file referencescore::MeshIndex<dimension>and the direction constantsdirX,dirY, anddirZ, all provided bycore/utilities/index/index.hpp.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
33-45:⚠️ Potential issueMissing standard headers for
std::isnanandstd::numeric_limits
magneticRegriding_uses bothstd::isnanandstd::numeric_limits::quiet_NaN()but<cmath>and<limits>are not included.
This will break strict or pre-compiled-header builds.#include <iostream> #include <string> +#include <cmath> +#include <limits>
♻️ Duplicate comments (2)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (1)
122-128: Modulo-based oddness tests still vulnerable for negative indicesPrevious reviews already asked for a helper (
isOdd) or bitwise check so the test is well-defined for negative values.
The patternix % 2 == 1appears repeatedly here; please replace with a consistent utility orix & 1.Also applies to: 136-144, 161-168, 186-194, 240-248, 302-309
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
200-208:⚠️ Potential issueCheck
createScheduleresults before storing
SAMRAI::xfer::RefineAlgorithm::createSchedulemay returnnullptr(e.g. empty level, missing variables).
Dereferencing later without verification will seg-fault.- magSharedNodeRefineSchedules[levelNumber] - = BalgoNode.createSchedule(level, &magneticRefinePatchStrategy_); +auto sharedNodeSched = BalgoNode.createSchedule(level, &magneticRefinePatchStrategy_); +if (!sharedNodeSched) + throw std::runtime_error("Failed to create magnetic shared-node schedule for level " + + std::to_string(levelNumber)); +magSharedNodeRefineSchedules[levelNumber] = sharedNodeSched;Apply the same guard to the two other schedule creations in this block.
🧹 Nitpick comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
777-803:numeric_limits<double>::quiet_NaN()in hot loop – cache value
quiet_NaN()is a constexpr but some libstdc++ implementations call a helper each time.
Fetching it once per patch avoids thousands of redundant calls:const double NaN = std::numeric_limits<double>::quiet_NaN(); ... B(idx) = NaN;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/amr/data/field/field_geometry.hpp(2 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)tests/core/numerics/interpolator/test_main.cpp(1 hunks)tests/functional/harris/harris_2d.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/data/field/field_geometry.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/functional/harris/harris_2d.py
- tests/core/numerics/interpolator/test_main.cpp
- 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.
src/amr/data/field/refine/magnetic_refine_patch_strategy.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧠 Learnings (1)
src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (2)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/amr/level_initializer/hybrid_level_initializer.hpp:44-49
Timestamp: 2024-09-17T13:36:04.487Z
Learning: In this codebase, when a derived class method overrides a virtual function using `override`, and there are no further subclasses, the `virtual` keyword is not required in the subclass.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/amr/level_initializer/hybrid_level_initializer.hpp:44-49
Timestamp: 2024-10-09T08:32:15.667Z
Learning: In this codebase, when a derived class method overrides a virtual function using `override`, and there are no further subclasses, the `virtual` keyword is not required in the subclass.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
UCaromel
force-pushed
the
refine-patch-strategy
branch
2 times, most recently
from
b2f2c87 to
c2bdb27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
204-213:createScheduleresult still unchecked – possible null dereference
The earlier review already highlighted that schedule creation can legitimately fail and returnnullptr. The map is still populated without validation, so a laterfillData()will seg-fault.Please add the null-check suggested previously or propagate an exception when creation fails.
🧹 Nitpick comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
155-164: Avoid repeated heap allocations of identicalVariableFillPatternobjects
X/Y/ZVariableFillPatterninstances are created every timeregisterQuantitiesis called, yet they carry no state. This results in avoidable heap traffic and fragmentation.-std::shared_ptr<SAMRAI::xfer::VariableFillPattern> xVariableFillPattern - = std::make_shared<XVariableFillPattern>(); +static const std::shared_ptr<SAMRAI::xfer::VariableFillPattern> xVariableFillPattern = + std::make_shared<XVariableFillPattern>();Do the same for Y and Z to reuse a single instance per process.
781-807: Costly per-cellstd::isnanscan on every regridThe loop first poisons every magnetic ghost cell with NaNs and immediately scans them again to post-process. This doubles memory traffic and CPU cycles for large patches.
A tighter approach: compute the “needs-postprocess” condition directly while refining (or mark cells via a bool buffer) instead of the NaN pattern.
If clarity is preferred over speed, add a
TODO performancecomment so the intent is explicit.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyphare/pyphare/core/gridlayout.py(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/core/data/field/field.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)tests/core/numerics/interpolator/test_main.cpp(1 hunks)tests/functional/harris/harris_2d.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/data/field/field.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/functional/harris/harris_2d.py
- tests/core/numerics/interpolator/test_main.cpp
- pyphare/pyphare/core/gridlayout.py
- src/core/data/grid/gridlayout.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
| magGhostsRefineSchedules[levelNumber] = Balgo.createSchedule( | ||
| level, levelNumber - 1, hierarchy, &magneticRefinePatchStrategy_); | ||
|
|
There was a problem hiding this comment.
levelNumber - 1 is −1 for the root level – undefined behaviour in SAMRAI
When levelNumber == 0 the call
Balgo.createSchedule(level, levelNumber - 1, hierarchy, &magneticRefinePatchStrategy_);passes -1 as the coarser-level index. SAMRAI APIs expect a valid level id or a nullptr/overload indicating “no coarser level”. Relying on -1 is undocumented and could crash or silently skip refinement.
Consider guarding the root case:
if (levelNumber != rootLevelNumber) {
magGhostsRefineSchedules[levelNumber] =
Balgo.createSchedule(level, levelNumber - 1, hierarchy, &magneticRefinePatchStrategy_);
}or use the overload that takes a nullptr for the coarser level.
🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp around lines 210 to
212, the call to Balgo.createSchedule uses levelNumber - 1 as the coarser-level
index, which becomes -1 at the root level, causing undefined behavior. Fix this
by adding a condition to check if levelNumber is not the root level before
calling createSchedule with levelNumber - 1, or alternatively use the overload
of createSchedule that accepts nullptr to indicate no coarser level for the
root.
|
For regrid, as of this PR, we opted for a solution were we first set the entire level to NaN, then run the magnetic refinement, that give their values to the new fine faces shared with coarse, and then copies all the faces that were on the previous fine level onto the new fine level. We then fill the remaining faces (still at NaN) using the values newly refined on the fine shared coarse-fine faces. |
adding the patch refine strategy for the magnetic field forcing even number of ghost cells 3D for magnetic regriding the path of the SAMRAI cleanup and comments back to NaNs 3d fixes + refactoring back to iterating on the fine field box (in mpi the fine_box might be different from the patch box)
UCaromel
force-pushed
the
refine-patch-strategy
branch
from
c2bdb27 to
c14c9f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
41-46:<limits>header is required forstd::numeric_limits
std::numeric_limits<double>::quiet_NaN()is used later in the file (≈ l. 795) but<limits>is not included.
Add the missing header to avoid compilation failures on some toolchains.#include <iterator> #include <optional> #include <utility> #include <iomanip> #include <iostream> #include <string> +#include <limits>
♻️ Duplicate comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
204-212: Missing null-check aftercreateSchedule– previous warning still applies
BalgoNode.createSchedule/Balgo.createSchedulecan legally return a nullshared_ptron failure, yet the result is stored unconditionally and later dereferenced (e.g. inpostSynchronize).
Please guard against nullptr and throw/log an error as already suggested in an earlier review.
210-212:levelNumber - 1passes ‑1 for the root level
InvokingcreateSchedule(..., levelNumber - 1, …)whilelevelNumber==0yields ‑1, which is outside the valid range for SAMRAI level indices and causes UB.
Add an explicit root-level branch or use the overload that accepts anullptrcoarser level.
🧹 Nitpick comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
53-67: Empty*VariableFillPatternsubclasses – consider using single stateless instances
The three subclasses are empty and identical; instantiating them repeatedly wastes heap allocations.
You could declare oneconstexpr/static object per class or even one templated alias, avoiding dynamic allocation:static XVariableFillPattern xFillPattern{}; static YVariableFillPattern yFillPattern{}; static ZVariableFillPattern zFillPattern{}; auto xVariableFillPattern = std::shared_ptr<SAMRAI::xfer::VariableFillPattern>(&xFillPattern, [](void*){});
795-806: Potential performance hit: triple ghost-box traversal doing identical work
layout.evalOnGhostBoxis called three times (once per component) to set NaNs, each iterating over the same ghost region.
You could fuse the loops to cut traversal cost by ~3×, especially relevant on large 3-D patches.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pyphare/pyphare/core/gridlayout.py(1 hunks)src/amr/data/field/field_data.hpp(1 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/amr/messengers/refiner.hpp(1 hunks)src/core/data/field/field.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)tests/core/numerics/interpolator/test_main.cpp(1 hunks)tests/functional/harris/harris_2d.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/core/data/field/field.hpp
- src/amr/messengers/refiner.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/functional/harris/harris_2d.py
- src/amr/data/field/field_data.hpp
- src/core/data/grid/gridlayout.hpp
- tests/core/numerics/interpolator/test_main.cpp
- src/amr/data/field/refine/magnetic_field_refiner.hpp
- pyphare/pyphare/core/gridlayout.py
- src/amr/data/field/field_variable_fill_pattern.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.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
932-971:PHARE_DEBUG_DOguard hides runtime NaN checks in release builds
When compiled without debug, silent NaNs could propagate. Consider enabling the final NaN verification unconditionally (or at least behind a less aggressive compile-time flag).
3 participants
This PR implements the divergence free refinement formulas from Toth and Roe (2002). It uses samrai's RefinePatchStrategy, which allows us to add preprocessing and post-processing functionalities on creation of a schedule. We are able to use the post-processing to code these formulas because the values of the new fine faces (that don't share their location with old coarse faces) only need the information from the previously refined faces that shared their location with the old coarse faces.
Note: