fine_boundary_represents_var for copy **before** refinement #1062
Conversation
nicolasaunai
added
bug 🔥
📝 WalkthroughWalkthroughThis update introduces NaN-initialization for floating-point data arrays and grids, with new constructors and concepts enforcing this behavior. It refactors ghost cell filling and refinement routines to avoid overwriting non-NaN values, and updates interfaces to pass full patch level objects instead of level numbers. Additional debug output and zero-initialization are added for improved diagnostics and test reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Grid
participant NdArrayVector
User->>Grid: Construct with floating-point type
Grid->>NdArrayVector: Initialize internal vector with NaN
NdArrayVector-->>Grid: Returns NaN-initialized array
Grid-->>User: Returns NaN-initialized grid
sequenceDiagram
participant Solver
participant HybridMessenger
participant HybridMessengerStrategy
participant PatchLevel
Solver->>HybridMessenger: fillElectricGhosts(E, PatchLevel, fillTime)
HybridMessenger->>HybridMessengerStrategy: fillElectricGhosts(E, PatchLevel, fillTime)
HybridMessengerStrategy->>PatchLevel: Set NaN on ghost nodes
HybridMessengerStrategy->>PatchLevel: Fill electric ghosts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🪛 Ruff (0.12.2)tests/simulator/test_advance.py290-290: Remove unused import: (F401) 306-306: Ambiguous variable name: (E741) ⏰ 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 (1)
✨ Finishing Touches
🧪 Generate unit tests
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
|
|
(executed on test/functional/harris_2d.py modified with 20 nx cells for it to be fast) Breakpoint in RefineAlgorithm::registerRefine (for Balgo) indeed shows the internal boolean set to false now: 
Breakpoint at:
continue : first hits copy() 
continue: then hits refine() 
|
|
MPI test fails : test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts_5 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/simulator/test_advance.py (3)
290-291: Remove unused matplotlib.pyplot import.The
matplotlib.pyplotimport is not used in the current code. OnlyRectanglefrommatplotlib.patchesis needed.- import matplotlib.pyplot as plt - from matplotlib.patches import Rectangle + from matplotlib.patches import Rectangle
293-300: Improve variable naming for better readability.The variable
lin the list comprehension is ambiguous. Use more descriptive names for better code clarity.def makerec(lower, upper, dl, fc="none", ec="g", lw=1, ls="-"): origin = (lower[0] * dl[0], lower[1] * dl[1]) sizex, sizey = [ - (u - l) * d for u, l, d in zip(upper, lower, dl) + (u - low) * d for u, low, d in zip(upper, lower, dl) ] return Rectangle( origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw )
302-330: Excellent debugging enhancement with minor suggestions.The comprehensive visualization code provides valuable debugging context for field overlap failures. The implementation correctly highlights failing overlap boxes and generates descriptive plots.
Consider these improvements:
Extract visualization logic: Consider moving this debugging code to a separate method to improve readability and reusability.
Add dimensionality check: The code assumes 2D data when accessing
pd1.layout.dl. Add a check for the data dimensionality:if len(pd1.layout.dl) >= 2: # Ensure 2D or higher # existing plotting code else: print("Visualization skipped: only supports 2D+ data")
- Optional conditional execution: Consider adding a flag to control when expensive plotting occurs:
if self.debug_plots: # or environment variable check # plotting code
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/simulator/test_advance.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
296-296: Ambiguous variable name: l
(E741)
⏰ 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 (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
| # if self.rethrow_: | ||
| # raise e | ||
| # return diff_boxes(slice1, slice2, box) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making error handling behavior configurable.
Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.
Consider making this behavior configurable:
- # if self.rethrow_:
- # raise e
- # return diff_boxes(slice1, slice2, box)
+ if getattr(self, 'rethrow_', True):
+ raise e
+ return diff_boxes(slice1, slice2, box)This allows debugging mode when needed while preserving the original test semantics by default. You could set self.rethrow_ = False in specific test cases or via environment variables when debugging is required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # if self.rethrow_: | |
| # raise e | |
| # return diff_boxes(slice1, slice2, box) | |
| if getattr(self, 'rethrow_', True): | |
| raise e | |
| return diff_boxes(slice1, slice2, box) |
🤖 Prompt for AI Agents
In tests/simulator/test_advance.py around lines 343 to 345, the error handling
code is commented out, changing the behavior from fail-fast to log-and-continue.
To fix this, introduce a configurable flag such as self.rethrow_ that controls
whether to re-raise the exception or continue after logging. Use this flag to
conditionally raise the error or return diff_boxes, allowing tests to run in
either fail-fast or debug mode based on configuration or environment variables.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/simulator/test_advance.py (1)
353-355: Consider making error handling behavior configurable.Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.
Consider making this behavior configurable:
- # if self.rethrow_: - # raise e - # return diff_boxes(slice1, slice2, box) + if getattr(self, 'rethrow_', True): + raise e + return diff_boxes(slice1, slice2, box)This allows debugging mode when needed while preserving the original test semantics by default. You could set
self.rethrow_ = Falsein specific test cases or via environment variables when debugging is required.
🧹 Nitpick comments (3)
tests/simulator/test_advance.py (3)
290-292: Fix unused import and improve import organization.The
matplotlib.pyplotimport is unused since the code usesdatahier.plot()method instead. Additionally, consider moving these imports to the top of the method or using conditional imports.- import matplotlib.pyplot as plt - from matplotlib.patches import Rectangle + from matplotlib.patches import Rectangle
298-307: Fix ambiguous variable name in lambda function.The lambda function uses an ambiguous variable name
lwhich reduces code readability.- def makerec( - lower, upper, dl, fc="none", ec="g", lw=1, ls="-" - ): - origin = (lower[0] * dl[0], lower[1] * dl[1]) - sizex, sizey = [ - (u - l) * d for u, l, d in zip(upper, lower, dl) - ] - return Rectangle( - origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw - ) + def makerec( + lower, upper, dl, fc="none", ec="g", lw=1, ls="-" + ): + origin = (lower[0] * dl[0], lower[1] * dl[1]) + sizex, sizey = [ + (upper_val - lower_val) * delta for upper_val, lower_val, delta in zip(upper, lower, dl) + ] + return Rectangle( + origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw + )
293-342: Consider adding 1D and 3D debugging support.The debugging visualization code only handles 2D cases. Consider adding support for other dimensions or at least documenting this limitation.
if box.ndim == 2: failed_i, failed_j = np.where( np.abs(slice1 - slice2) > 5.5e-15 ) + elif box.ndim == 1: + failed_i = np.where(np.abs(slice1 - slice2) > 5.5e-15)[0] + print(f"1D debugging: failed indices: {failed_i}") + else: + print(f"Debugging visualization not implemented for {box.ndim}D")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/simulator/advance/test_fields_advance_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
303-303: Ambiguous variable name: l
(E741)
⏰ 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 (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (1)
tests/simulator/test_advance.py (1)
290-355: Confirm debug instrumentation isolates MPI overlap discrepancyThe added
Rectangleoverlays and print statements intests/simulator/test_advance.pyare self-contained (no other MPI-overlap debug patterns detected). Please validate that:
- The generated PNGs clearly highlight the exact failing cells and hierarchy levels.
- The console logs (
ghost_box,box,interp_order,coarsest_time,failed_i/j) provide enough context to reproduce and pinpoint the numerical mismatch.After you’ve identified the root cause, consider guarding or removing these heavy debugging blocks behind a verbose/debug flag.
| smallest_patch_size=smallest_patch_size + 1, | ||
| largest_patch_size=smallest_patch_size + 1, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the rationale for increasing patch size parameters.
The patch size parameters have been increased by 1 unit for both smallest_patch_size and largest_patch_size. This change affects the hierarchy setup used in the overlap testing.
Please clarify the reasoning behind this adjustment and verify that it's necessary for the fine_boundary_represents_var changes. Run the following script to check if this change is related to the reported MPI test failure:
🏁 Script executed:
#!/bin/bash
# Description: Search for references to patch size parameters and related test failures
# Check for other patch size adjustments in the codebase
rg -A 3 -B 3 "smallest_patch_size.*\+.*1|largest_patch_size.*\+.*1"
# Look for test failure reports or comments about this specific test
rg -A 5 -B 5 "test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts"
# Search for any comments or documentation about patch size requirements
rg -A 3 -B 3 "patch.*size.*ghost|ghost.*patch.*size"Length of output: 7513
🏁 Script executed:
#!/bin/bash
# Display the first 100 lines to inspect how patch sizes are set up
head -n 100 tests/simulator/advance/test_fields_advance_2d.pyLength of output: 3102
🏁 Script executed:
#!/bin/bash
# Locate and display the definition of check_patch_size
rg -n "def check_patch_size" -A 10 -B 3 pyphare/pyphare/pharein/simulation.pyLength of output: 585
🏁 Script executed:
#!/bin/bash
# Display the full check_patch_size function for context
sed -n '380,450p' pyphare/pyphare/pharein/simulation.pyLength of output: 2915
🏁 Script executed:
#!/bin/bash
# Display lines 450-550 to capture the end of check_patch_size
sed -n '450,550p' pyphare/pyphare/pharein/simulation.pyLength of output: 3661
Clarify and correct patch-size assignment in the test
It looks like in test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts you’re doing:
largest_patch_size, smallest_patch_size = check_patch_size(...)
datahier = self.getHierarchy(
…,
smallest_patch_size=smallest_patch_size + 1,
largest_patch_size=smallest_patch_size + 1,
…
)– check_patch_size returns (largest_patch_size, smallest_patch_size), but here the first returned value (which is always None when not passed in) is assigned to largest_patch_size and discarded.
– You then use the minimal patch size for both smallest and largest, and bump it by an extra +1.
Please:
- Confirm that you intentionally ignore the computed largest_patch_size and use the minimal patch size for both parameters.
- Explain why an additional
+1(beyond the built-in bump in check_patch_size) is required for this test. - For clarity, consider unpacking as
_, min_ps = check_patch_size(...)and then passingmin_ps + 1to both parameters or use the actual returned largest_patch_size if you intended different min/max values. - Add a code comment above the call to document the reason for the extra
+1so future readers understand why the test needs a patch size two cells larger than the max ghost width.
🤖 Prompt for AI Agents
In tests/simulator/advance/test_fields_advance_2d.py around lines 70 to 71, the
unpacking of check_patch_size's return values is confusing and potentially
incorrect because largest_patch_size is assigned the first returned value but
then discarded, and smallest_patch_size is used for both smallest_patch_size and
largest_patch_size parameters with an added +1. To fix this, explicitly unpack
as _, min_ps = check_patch_size(...) to clarify ignoring the first value, then
pass min_ps + 1 to both smallest_patch_size and largest_patch_size parameters.
Add a comment above this call explaining why the extra +1 is necessary, such as
to ensure the patch size is two cells larger than the max ghost width for the
test's correctness.
|
Ok right now I think the problem is that if we have the copy done before the refinement and we have this configuration: patch B below patch A. then patch B top ghosts go up to patch A top border. patch B gets its ghosts from patch A, up to its top border. then patch A gets its top level ghost refined, including top border. This created a mismatch between A top border and B top ghost border. Note this mismatch disappears if A gets refined before B copies the values, thus consistent with master not failing. I do not think we can trust box clustering to never give us patches smaller or equal to the ghost size. What probably is TBD is that level ghost filling should probably never overwrite borders. A priori, border faces are solution values and thus if it didn't probably the test would pass. TBC. |
|
now also changed test_advance_field1d.py to have smallest patch size = ghost nbr+1 because that was also failing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/simulator/test_advance.py (1)
359-361: Consider making error handling behavior configurable.Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.
Consider making this behavior configurable:
- # if self.rethrow_: - # raise e - # return diff_boxes(slice1, slice2, box) + if getattr(self, 'rethrow_', True): + raise e + return diff_boxes(slice1, slice2, box)This allows debugging mode when needed while preserving the original test semantics by default. You could set
self.rethrow_ = Falsein specific test cases or via environment variables when debugging is required.
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
301-310: Fix ambiguous variable name and improve code clarity.The variable name
lin the list comprehension is ambiguous and flagged by static analysis. Also consider adding explicit handling for 3D cases.Apply this diff to fix the ambiguous variable name:
- sizex, sizey = [ - (u - l) * d for u, l, d in zip(upper, lower, dl) - ] + sizex, sizey = [ + (u - low) * d for u, low, d in zip(upper, lower, dl) + ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/simulator/advance/test_fields_advance_1d.py
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
306-306: Ambiguous variable name: l
(E741)
🔇 Additional comments (1)
tests/simulator/test_advance.py (1)
290-292: Conditional imports are appropriate here.The matplotlib imports are correctly placed inside the exception handler to avoid loading heavy dependencies unless debugging is needed. The static analysis warning about unused
matplotlib.pyplotis a false positive - the import is used implicitly through thedatahier.plot()calls later in the code.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/simulator/test_advance.py (1)
394-396: Consider making error handling behavior configurable.The commented-out error re-throwing significantly changes test behavior from fail-fast to log-and-continue, potentially masking legitimate test failures. This matches a previous review comment about the same issue.
While the enhanced debugging information is valuable, consider making this behavior configurable:
- # if self.rethrow_: - # raise e - # return diff_boxes(slice1, slice2, box) + if getattr(self, 'rethrow_', True): + raise e + return diff_boxes(slice1, slice2, box)This allows debugging mode when needed while preserving the original test semantics by default.
🧹 Nitpick comments (3)
src/amr/data/field/field_variable_fill_pattern.hpp (1)
107-120: Consider optimizing the dynamic casting and improve code clarity.The conditional logic correctly implements the intended behavior, but there are some concerns:
Performance: Two separate
dynamic_castoperations are performed whenoverwrite_interior_is false, which could be expensive in performance-critical paths.Code clarity: The variable names
geom,basic_overlap, andcastedcould be more descriptive.Consider this refactor to optimize performance and improve readability:
- auto geom = pdf.getBoxGeometry(patch_box); - auto basic_overlap - = pdf.getBoxGeometry(patch_box)->setUpOverlap(overlap_boxes, transformation); + auto geom = pdf.getBoxGeometry(patch_box); + auto basic_overlap = geom->setUpOverlap(overlap_boxes, transformation); if (overwrite_interior_) return basic_overlap; - auto& overlap = dynamic_cast<FieldOverlap const&>(*basic_overlap); - auto destinationBoxes = overlap.getDestinationBoxContainer(); - auto& casted = dynamic_cast<FieldGeometryBase<dimension> const&>(*geom); - destinationBoxes.removeIntersections(casted.interiorFieldBox()); + auto& field_overlap = dynamic_cast<FieldOverlap const&>(*basic_overlap); + auto destinationBoxes = field_overlap.getDestinationBoxContainer(); + auto& field_geometry = dynamic_cast<FieldGeometryBase<dimension> const&>(*geom); + destinationBoxes.removeIntersections(field_geometry.interiorFieldBox()); - return std::make_shared<FieldOverlap>(destinationBoxes, overlap.getTransformation()); + return std::make_shared<FieldOverlap>(destinationBoxes, field_overlap.getTransformation());This eliminates the redundant
getBoxGeometrycall and uses more descriptive variable names.tests/simulator/test_advance.py (2)
290-292: Remove unused matplotlib.pyplot import.The
matplotlib.pyplotimport is unused according to static analysis. The code only usesRectanglefrommatplotlib.patches.- import matplotlib.pyplot as plt - from matplotlib.patches import Rectangle + from matplotlib.patches import Rectangle
293-380: Consider extracting visualization logic to a helper method.The debugging visualization code is quite extensive and could benefit from being extracted to a separate helper method to improve maintainability and readability of the main test method.
Consider creating a helper method like
_create_overlap_failure_plots(self, pd1, pd2, box, failed_indices, ilvl, datahier)to encapsulate this visualization logic. This would:
- Reduce the complexity of the main test method
- Make the visualization code reusable
- Improve code organization and readability
- Make the test method easier to understand and maintain
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/amr/data/field/field_geometry.hpp(1 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/advance/test_fields_advance_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/amr/data/field/field_geometry.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/simulator/advance/test_fields_advance_2d.py
- tests/simulator/advance/test_fields_advance_1d.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/data/field/field_variable_fill_pattern.hpp
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
306-306: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
| def makerec( | ||
| lower, upper, dl, fc="none", ec="g", lw=1, ls="-" | ||
| ): | ||
| origin = (lower[0] * dl[0], lower[1] * dl[1]) | ||
| sizex, sizey = [ | ||
| (u - l) * d for u, l, d in zip(upper, lower, dl) | ||
| ] | ||
| return Rectangle( | ||
| origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix ambiguous variable name and improve function clarity.
The makerec function uses ambiguous variable names like l which violates naming conventions and reduces readability.
def makerec(
- lower, upper, dl, fc="none", ec="g", lw=1, ls="-"
+ lower, upper, spacing, fc="none", ec="g", lw=1, ls="-"
):
- origin = (lower[0] * dl[0], lower[1] * dl[1])
+ origin = (lower[0] * spacing[0], lower[1] * spacing[1])
sizex, sizey = [
- (u - l) * d for u, l, d in zip(upper, lower, dl)
+ (u - lower_val) * spacing_val
+ for u, lower_val, spacing_val in zip(upper, lower, spacing)
]
return Rectangle(
origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def makerec( | |
| lower, upper, dl, fc="none", ec="g", lw=1, ls="-" | |
| ): | |
| origin = (lower[0] * dl[0], lower[1] * dl[1]) | |
| sizex, sizey = [ | |
| (u - l) * d for u, l, d in zip(upper, lower, dl) | |
| ] | |
| return Rectangle( | |
| origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw | |
| ) | |
| def makerec( | |
| lower, upper, spacing, fc="none", ec="g", lw=1, ls="-" | |
| ): | |
| origin = (lower[0] * spacing[0], lower[1] * spacing[1]) | |
| sizex, sizey = [ | |
| (u - lower_val) * spacing_val | |
| for u, lower_val, spacing_val in zip(upper, lower, spacing) | |
| ] | |
| return Rectangle( | |
| origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
306-306: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
In tests/simulator/test_advance.py around lines 301 to 310, the function makerec
uses ambiguous variable names such as 'l' in the list comprehension. Rename
these variables to more descriptive names like 'lower_bound' or 'lower_val' to
improve readability and adhere to naming conventions. Update all occurrences
within the function accordingly to maintain clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/simulator/test_advance.py (2)
402-404: Consider making error handling behavior configurable.Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.
301-310: Fix ambiguous variable name and improve function clarity.The
makerecfunction uses ambiguous variable names likelwhich violates naming conventions and reduces readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/amr/data/field/field_data.hpp(1 hunks)src/amr/data/field/field_variable.hpp(1 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(2 hunks)src/amr/data/field/refine/field_refine_operator.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(2 hunks)src/amr/solvers/solver_ppc.hpp(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(2 hunks)tests/simulator/advance/test_fields_advance_2d.py(2 hunks)tests/simulator/test_advance.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/amr/solvers/solver_ppc.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/simulator/advance/test_fields_advance_1d.py
- src/amr/data/field/field_variable.hpp
- tests/simulator/advance/test_fields_advance_2d.py
- src/amr/data/field/field_variable_fill_pattern.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/data/field/refine/field_refine_operator.hppsrc/amr/data/field/field_data.hpp
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
306-306: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
| std::cout << "copy_ debug all boxes: " | ||
| << "sourceBox: " << sourceBox | ||
| << ", destinationBox: " << destinationBox | ||
| << ", transformedSource: " << transformedSource | ||
| << ", intersectionBox: " << intersectionBox << std::endl; |
There was a problem hiding this comment.
Remove or properly implement debug output
Debug print statements should not be committed to production code. This file already uses the PHARE logging framework (e.g., PHARE_LOG_SCOPE at line 115).
- std::cout << "copy_ debug all boxes: "
- << "sourceBox: " << sourceBox
- << ", destinationBox: " << destinationBox
- << ", transformedSource: " << transformedSource
- << ", intersectionBox: " << intersectionBox << std::endl;
+ // If debug output is needed, use PHARE logging:
+ // PHARE_LOG_SCOPE(4, "FieldData::copy_::debug_boxes");
+ // PHARE_LOG_LINE_STR("sourceBox: " << sourceBox
+ // << ", destinationBox: " << destinationBox
+ // << ", transformedSource: " << transformedSource
+ // << ", intersectionBox: " << intersectionBox);🤖 Prompt for AI Agents
In src/amr/data/field/field_data.hpp around lines 401 to 405, remove the
std::cout debug print statements or replace them with appropriate PHARE logging
calls consistent with the existing logging framework used in the file. This
ensures debug output is managed uniformly and not left as raw console prints in
production code.
| std::cout << "debug refine operator: " | ||
| << "destinationFieldBox: " << destFieldBox | ||
| << ", sourceFieldBox: " << sourceFieldBox << ", box: " << box | ||
| << ", intersectionBox: " << intersectionBox << std::endl; |
There was a problem hiding this comment.
Remove or properly implement debug output
Debug print statements should not be committed to production code. Consider either:
- Removing this debug output entirely, or
- Using the PHARE logging framework with appropriate log levels
- std::cout << "debug refine operator: "
- << "destinationFieldBox: " << destFieldBox
- << ", sourceFieldBox: " << sourceFieldBox << ", box: " << box
- << ", intersectionBox: " << intersectionBox << std::endl;
+ // If debug output is needed, use PHARE logging:
+ // PHARE_LOG_SCOPE(4, "refine_operator_debug");
+ // PHARE_LOG_LINE_STR("destinationFieldBox: " << destFieldBox
+ // << ", sourceFieldBox: " << sourceFieldBox
+ // << ", box: " << box
+ // << ", intersectionBox: " << intersectionBox);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::cout << "debug refine operator: " | |
| << "destinationFieldBox: " << destFieldBox | |
| << ", sourceFieldBox: " << sourceFieldBox << ", box: " << box | |
| << ", intersectionBox: " << intersectionBox << std::endl; | |
| // If debug output is needed, use PHARE logging: | |
| // PHARE_LOG_SCOPE(4, "refine_operator_debug"); | |
| // PHARE_LOG_LINE_STR("destinationFieldBox: " << destFieldBox | |
| // << ", sourceFieldBox: " << sourceFieldBox | |
| // << ", box: " << box | |
| // << ", intersectionBox: " << intersectionBox); |
🤖 Prompt for AI Agents
In src/amr/data/field/refine/field_refine_operator.hpp around lines 111 to 114,
the code contains a std::cout debug print statement that should not be in
production code. Remove this debug output entirely or replace it with a call to
the PHARE logging framework using an appropriate log level to ensure proper
logging without cluttering standard output.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
407-426: Document the NaN initialization strategy.The ghost layer NaN initialization appears to be implementing the "copy before refinement" behavior. While the implementation looks correct, it would benefit from a comment explaining why ghost values are set to NaN before refinement.
Add a comment before the loop:
// Initialize ghost layer values to NaN before refinement. // This ensures we can distinguish between values that were copied // vs. refined when fine_boundary_represents_var is false. for (auto& comp : E) { // ... existing code }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/amr/data/field/field_variable.hpp(1 hunks)src/amr/data/field/refine/electric_field_refiner.hpp(8 hunks)src/amr/data/field/refine/field_refiner.hpp(3 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/hybrid_messenger.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/amr/data/field/field_variable.hpp
- src/amr/solvers/solver_ppc.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/messengers/mhd_hybrid_messenger_strategy.hppsrc/amr/data/field/refine/field_refiner.hppsrc/amr/messengers/hybrid_messenger.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/hybrid_messenger_strategy.hppsrc/amr/data/field/refine/electric_field_refiner.hpp
🧬 Code Graph Analysis (2)
src/amr/level_initializer/hybrid_level_initializer.hpp (3)
src/amr/messengers/hybrid_messenger.hpp (4)
E(270-274)E(270-271)E(331-331)E(331-331)src/amr/messengers/hybrid_messenger_strategy.hpp (1)
E(70-71)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (8)
E(392-428)E(392-393)level(600-625)level(600-603)level(730-742)level(730-730)level(866-881)level(866-866)
src/amr/data/field/refine/field_refiner.hpp (2)
src/amr/data/field/refine/electric_field_refiner.hpp (6)
fineIndex(62-65)fineIndex(62-62)fineIndex(66-69)fineIndex(66-66)fineIndex(70-73)fineIndex(70-70)src/amr/data/field/coarsening/field_coarsen_index_weight.hpp (2)
fineIndex(89-92)fineIndex(93-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (6)
src/amr/level_initializer/hybrid_level_initializer.hpp (1)
167-167: LGTM! Interface update aligns with broader refactoring.The change from passing
levelNumbertolevelis consistent with the updatedfillElectricGhostsmethod signature that now accepts aSAMRAI::hier::PatchLevelobject. This provides the method with richer context about the patch level structure.src/amr/data/field/refine/field_refiner.hpp (1)
94-95: Excellent defensive programming approach!The addition of NaN checks before field assignment is crucial for the PR's objective of ensuring copy occurs before refinement. This prevents overwriting valid fine field data that may have been copied earlier in the process.
The implementation is consistent across all dimensional cases (1D, 2D, 3D), which ensures uniform behavior throughout the refinement process.
Also applies to: 123-124, 162-164
src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (1)
87-87: Interface update maintains consistency.The parameter change from
int levelNumbertoSAMRAI::hier::PatchLevel const& levelaligns with the broader interface refactoring across messenger strategies. The empty method body suggests this strategy either doesn't need electric ghost filling or defers implementation.src/amr/data/field/refine/electric_field_refiner.hpp (1)
97-98: Comprehensive NaN protection implemented correctly.The systematic addition of NaN checks before all electric field assignments is excellent defensive programming that aligns perfectly with the PR's objective. This ensures that:
- Valid data copied before refinement is never overwritten
- The Fujimoto et al. 2011 electric field refinement strategy is preserved
- All dimensional cases and field components are consistently protected
The implementation is thorough and covers all assignment paths in the electric field refinement logic.
Also applies to: 123-124, 130-132, 146-147, 153-155, 164-165, 169-172, 175-178, 181-186, 215-216, 223-225, 232-234, 240-245, 257-258, 270-272, 279-281, 288-293, 305-306, 313-315, 322-324, 331-336
src/amr/messengers/hybrid_messenger_strategy.hpp (1)
70-71: Foundational interface improvement.Updating the pure virtual method signature to accept a
SAMRAI::hier::PatchLevel const& levelinstead of an integer provides implementing classes with richer context for electric ghost filling operations. This change drives the consistent interface updates across all messenger strategy implementations.The enhanced interface enables more sophisticated patch-level-aware ghost filling logic while maintaining clean abstraction.
src/amr/messengers/hybrid_messenger.hpp (1)
270-274: fillElectricGhosts signature is now consistent across the codebaseVerified all occurrences of
fillElectricGhostsvia high-level search; every definition and call uses
SAMRAI::hier::PatchLevel const& levelinstead of anint levelNumber, with no legacy signatures remaining.All set to merge.
| #include "amr/data/particles/particles_variable_fill_pattern.hpp" | ||
| #include "amr/data/field/time_interpolate/field_linear_time_interpolate.hpp" | ||
|
|
||
| #include "amr/resources_manager/amr_utils.hpp" |
There was a problem hiding this comment.
Remove duplicate include.
The header amr/resources_manager/amr_utils.hpp is already included on line 18. This duplicate include should be removed.
-#include "amr/resources_manager/amr_utils.hpp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "amr/resources_manager/amr_utils.hpp" |
🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp at line 33, remove
the duplicate include directive for "amr/resources_manager/amr_utils.hpp" since
it is already included at line 18. This will clean up the code and avoid
redundant includes.
| std::cout << "I am fucking regriding level " << levelNumber << " with oldLevel " | ||
| << (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null") | ||
| << " at time " << std::setprecision(16) << initDataTime << "\n"; |
There was a problem hiding this comment.
Remove debug output and use proper logging.
This debug output should be removed or replaced with proper logging using the PHARE logging framework. Additionally, the language used is inappropriate for production code.
- std::cout << "I am fucking regriding level " << levelNumber << " with oldLevel "
- << (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null")
- << " at time " << std::setprecision(16) << initDataTime << "\n";
+ PHARE_LOG_LINE_STR("Regridding level " + std::to_string(levelNumber)
+ + " with oldLevel " + (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null")
+ + " at time " + std::to_string(initDataTime));🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp around lines 282 to
284, remove the debug output that uses std::cout with inappropriate language.
Replace it with a proper logging call using the PHARE logging framework,
ensuring the message is professional and informative, including the levelNumber,
oldLevel status, and initDataTime with appropriate formatting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/simulator/test_advance.py (2)
301-307: Fix ambiguous variable name in list comprehension.The variable name
lin the list comprehension is ambiguous and violates naming conventions as noted in previous reviews.Apply the suggested fix from the previous review:
def makerec( - lower, upper, dl, fc="none", ec="g", lw=1, ls="-" + lower, upper, spacing, fc="none", ec="g", lw=1, ls="-" ): - origin = (lower[0] * dl[0], lower[1] * dl[1]) + origin = (lower[0] * spacing[0], lower[1] * spacing[1]) sizex, sizey = [ - (u - l) * d for u, l, d in zip(upper, lower, dl) + (u - lower_val) * spacing_val + for u, lower_val, spacing_val in zip(upper, lower, spacing) ]
402-404: Error handling behavior change needs careful consideration.As noted in the previous review, commenting out the error re-throwing fundamentally changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information for the specific issue mentioned in the PR (copy before refinement causing mismatches), it could mask other legitimate test failures.
The previous suggestion to make this configurable is still valid:
- # if self.rethrow_: - # raise e - # return diff_boxes(slice1, slice2, box) + if getattr(self, 'rethrow_', True): + raise e + return diff_boxes(slice1, slice2, box)This allows debugging mode when investigating the specific AMR refinement timing issues while preserving test integrity by default.
🧹 Nitpick comments (2)
src/core/data/ndarray/ndarray_vector.hpp (1)
273-273: Minor: Remove extra empty line.There's an unnecessary extra empty line that should be removed for consistency with the surrounding code formatting.
-tests/simulator/test_advance.py (1)
290-291: Remove unused matplotlib.pyplot import.The
matplotlib.pyplotimport is flagged as unused by static analysis, but examining the code shows it's actually used viapltin the exception handling block.The static analysis tool missed the
pltusage. However, consider moving the import closer to its usage for better code organization:- import matplotlib.pyplot as plt from matplotlib.patches import Rectangle + import matplotlib.pyplot as plt
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/data/ndarray/ndarray_vector.hpp(3 hunks)tests/simulator/test_advance.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/core/data/ndarray/ndarray_vector.hpp
🧠 Learnings (1)
src/core/data/ndarray/ndarray_vector.hpp (1)
Learnt from: PhilipDeegan
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.
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
306-306: Ambiguous variable name: l
(E741)
⏰ 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-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
🔇 Additional comments (6)
src/core/data/ndarray/ndarray_vector.hpp (5)
229-230: LGTM: Clean concept definition for floating-point types.The
FloatingPointconcept provides a clear and expressive way to constrain template parameters to floating-point types, improving code readability and maintainability.
242-248: LGTM: Proper NaN initialization for floating-point constructors.The variadic constructor correctly initializes floating-point data with NaN values, which aligns with the PR's objective of handling NaN values explicitly in field operations. The template parameter
Uwith defaultDataTypeand the constraint ensure type safety.
250-256: LGTM: Consistent NaN initialization for array-based constructor.The array-based constructor properly mirrors the variadic constructor's behavior, initializing floating-point data with NaN. The use of
std::accumulatewithstd::multiplies<int>is consistent with the existing pattern.
257-264: LGTM: Clean constraint application for non-floating-point types.The
requires(!FloatingPoint<DataType>)constraint properly excludes floating-point types from this constructor, ensuring clear separation between initialization strategies. The constructor maintains the original behavior for non-floating-point types.
266-271: LGTM: Consistent constraint application.The array-based constructor for non-floating-point types correctly applies the same constraint, maintaining consistency with the variadic constructor approach.
tests/simulator/test_advance.py (1)
290-404: Enhanced debugging capabilities look comprehensive.The extensive visual diagnostic additions provide valuable debugging information for AMR field overlap failures. The implementation correctly:
- Identifies failing cells with proper tolerance checking
- Creates visual plots with patch boundaries and ghost boxes
- Marks failing cells with distinct markers
- Saves diagnostic plots with descriptive filenames
- Provides detailed textual output for debugging
This will significantly aid in diagnosing the mismatch issues mentioned in the PR objectives.
| extra_diag_options["mode"] = "overwrite" | ||
| extra_diag_options["dir"] = diag_outputs | ||
| self.register_diag_dir_for_cleanup(diag_outputs) | ||
| # self.register_diag_dir_for_cleanup(diag_outputs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cleanup registration is disabled - potential resource leak.
Commenting out the cleanup registration may lead to accumulated test artifacts and disk space issues, especially in CI environments where many tests run sequentially.
Consider making cleanup configurable rather than completely disabling it:
- # self.register_diag_dir_for_cleanup(diag_outputs)
+ if not getattr(self, 'skip_cleanup', False):
+ self.register_diag_dir_for_cleanup(diag_outputs)This allows selective cleanup disabling for debugging while preserving the default behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # self.register_diag_dir_for_cleanup(diag_outputs) | |
| if not getattr(self, 'skip_cleanup', False): | |
| self.register_diag_dir_for_cleanup(diag_outputs) |
🤖 Prompt for AI Agents
In tests/simulator/test_advance.py at line 77, the cleanup registration call is
commented out, which can cause resource leaks by leaving test artifacts on disk.
To fix this, re-enable the cleanup registration but make it conditional based on
a configuration flag or environment variable. This way, cleanup runs by default
but can be disabled selectively for debugging without risking disk space issues
in CI.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/amr/physical_models/hybrid_model.hpp (1)
139-142: LGTM: Clear NaN handling with good documentation.The explicit zeroing of
state.Jwith detailed comments effectively prevents NaN propagation, particularly for the 1D case whereJxisn't updated by Ampère's law. The comment clearly explains the rationale and the placement after electromagnetic initialization is appropriate.Consider whether this dimension-specific initialization could be moved to the
state.Jconstruction/initialization itself to avoid the initialize-then-zero pattern, but the current approach is clear and functional.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/amr/physical_models/hybrid_model.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/physical_models/hybrid_model.hpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (1)
src/amr/physical_models/hybrid_model.hpp (1)
129-129: LGTM: Consistent resource management extension.Adding
state.Jto thesetOnPatchcall properly extends resource management to include the current density field, which is consistent with its usage elsewhere in the class (e.g., lines 162, 169) and supports the initialization improvements in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/core/numerics/interpolator/test_main.cpp (2)
534-536: Simplify the zero initialization syntax.The explicit zero initialization is good practice for ensuring deterministic test behavior. However, the syntax can be simplified.
Apply this diff to simplify the syntax:
- (*(&rho)).zero(); - (*(&rho_c)).zero(); - v.zero(); + rho.zero(); + rho_c.zero(); + v.zero();
710-712: Simplify the zero initialization syntax.Same as the 1D version, the explicit zero initialization is good practice but the syntax can be simplified.
Apply this diff to simplify the syntax:
- (*(&rho)).zero(); - (*(&rho_c)).zero(); - v.zero(); + rho.zero(); + rho_c.zero(); + v.zero();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/core/numerics/interpolator/test_main.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/amr/data/field/refine/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
tests/core/numerics/interpolator/test_main.cpp (2)
Learnt from: rochSmets
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.
Learnt from: PhilipDeegan
PR: #790
File: tests/core/data/electrons/test_electrons.cpp:329-329
Timestamp: 2024-10-09T08:32:15.667Z
Learning: The trailing comma in the TYPED_TEST_SUITE macro call is used intentionally to silence a specific warning in Google Test.
⏰ 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)
| extra_diag_options["mode"] = "overwrite" | ||
| extra_diag_options["dir"] = diag_outputs | ||
| self.register_diag_dir_for_cleanup(diag_outputs) | ||
| # self.register_diag_dir_for_cleanup(diag_outputs) |
There was a problem hiding this comment.
I was thinking maybe we could have some switch for this to keep the diags even on test pass
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
33-33: Remove duplicate include.The header
amr/resources_manager/amr_utils.hppis already included on line 18. This duplicate include should be removed.-#include "amr/resources_manager/amr_utils.hpp"
282-284: Remove debug output and use proper logging.This debug output should be removed or replaced with proper logging using the PHARE logging framework. Additionally, the language used is inappropriate for production code.
- std::cout << "I am fucking regriding level " << levelNumber << " with oldLevel " - << (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null") - << " at time " << std::setprecision(16) << initDataTime << "\n"; + PHARE_LOG_LINE_STR("Regridding level " + std::to_string(levelNumber) + + " with oldLevel " + (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null") + + " at time " + std::to_string(initDataTime));
408-408: Remove debug output.Replace
std::coutwith proper logging framework.- std::cout << "FILLING ELECTRIC GHOSTS\n"; + PHARE_LOG_LINE_STR("Filling electric ghosts");
410-439: Fix critical component access bug and remove unused variables.Multiple issues need to be addressed:
- Critical Bug: All three electric field components are incorrectly accessing
Component::X- Unused Variables: Ex, Ey, Ez variables are declared but never used
- Code Logic: The NaN initialization logic is sound but the component access must be fixed first
auto& Ex = E(core::Component::X); - auto& Ey = E(core::Component::X); - auto& Ez = E(core::Component::X); + auto& Ey = E(core::Component::Y); + auto& Ez = E(core::Component::Z);Since these variables are unused, consider removing them entirely:
auto box = patch->getBox(); auto _ = resourcesManager_->setOnPatch(*patch, E); - auto& Ex = E(core::Component::X); - auto& Ey = E(core::Component::X); - auto& Ez = E(core::Component::X); auto layout = layoutFromPatch<GridLayoutT>(*patch);
🧹 Nitpick comments (4)
src/core/data/grid/grid.hpp (3)
51-58: Review template parameter design and NaN initialization.The template parameter
U = value_typewithFloatingPoint<U>constraint seems redundant sinceUdefaults tovalue_type. Consider simplifying torequires FloatingPoint<value_type>.Also, verify that
static_cast<U>(std::nan(""))is the intended approach for NaN initialization.- template<FloatingPoint U = value_type, std::size_t dim> + template<std::size_t dim> + requires FloatingPoint<value_type> Grid(std::string const& name, PhysicalQuantity qty, std::array<std::uint32_t, dim> const& dims, - value_type value = static_cast<U>(std::nan(""))) + value_type value = std::numeric_limits<value_type>::quiet_NaN())
60-67: Consistent template parameter simplification needed.Same issue as the previous constructor - the template parameter
Uis redundant. Also consider usingstd::numeric_limitsfor NaN generation for better type safety.- template<FloatingPoint U = value_type, typename GridLayout_t> + template<typename GridLayout_t> + requires FloatingPoint<value_type> Grid(std::string const& name, GridLayout_t const& layout, PhysicalQuantity qty, - value_type value = static_cast<U>(std::nan(""))) + value_type value = std::numeric_limits<value_type>::quiet_NaN())
51-85: Potential constructor ambiguity concerns.The constructor overload resolution relies on SFINAE with
requiresclauses. While this should work correctly, consider whether the split between floating-point and non-floating-point constructors could create maintenance issues or unexpected behavior for users.Consider documenting this design decision in class comments to help future maintainers understand why constructors are split based on value type, and ensure that the initialization behavior is well-documented for library users.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
365-365: Use const reference for better performance.The hybridModel should be declared as a const reference since it's not being modified in this context.
- auto& hybridModel = static_cast<HybridModel&>(model); + auto const& hybridModel = static_cast<HybridModel const&>(model);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(5 hunks)src/core/data/grid/grid.hpp(3 hunks)src/core/data/ndarray/ndarray_vector.hpp(3 hunks)tests/core/data/ndarray/test_main.cpp(6 hunks)tests/core/data/tensorfield/test_tensorfield_fixtures.hpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/core/data/ndarray/test_main.cpp
- tests/core/numerics/ion_updater/test_updater.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/data/ndarray/ndarray_vector.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
tests/core/data/tensorfield/test_tensorfield_fixtures.hppsrc/core/data/grid/grid.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
🔇 Additional comments (7)
tests/core/data/tensorfield/test_tensorfield_fixtures.hpp (2)
17-20: Clear documentation of initialization behavior change.The added documentation clearly explains the rationale for using zero initialization instead of NaN in test fixtures for convenience. This helps future maintainers understand the design decision.
57-58: No remaining NaN expectations—explicit zero initialization is correctI’ve verified that the
Gridconstructor now accepts an explicit initial value and all tensor-field tests have been updated to pass0.instead of relying on a NaN default. A case-insensitive search fornan/isnanintests/core/data/tensorfield/only turned up an outdated comment; no tests actually check for NaN. You can disregard the previous concern about NaN initialization.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (5)
47-47: LGTM: Appropriate include for NaN operations.The
<cmath>include is correctly added to support thestd::numeric_limitsand NaN operations used throughout the file.
287-291: LGTM: Proper current density initialization.The code correctly iterates over patches and zeros the current density field J. This ensures consistent initial conditions during regridding operations.
369-373: LGTM: Consistent current density initialization.The code properly initializes the current density field J to zero after level initialization, maintaining consistency with the regridding method.
404-405: Method signature change looks correct.The change from accepting an integer level number to accepting a
PatchLevelreference is appropriate and aligns with the implementation that iterates over patches in the level.
439-439: Verify GhostRefinerPool::fill signature consistencyWe’ve located several calls to
fillElectricGhostsand direct uses ofelecGhostsRefiners_.fill(...)—some passing aPatchLeveland others an integer. Please confirm whetherGhostRefinerPool::fillnow expects aSAMRAI::hier::PatchLevel const&(instead of an integer level number) and update all call sites accordingly:• src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
– Line 311:elecGhostsRefiners_.fill(E, levelNumber, initDataTime);
– Line 439:elecGhostsRefiners_.fill(E, level.getLevelNumber(), fillTime);
– Line 723:elecGhostsRefiners_.fill(hybridModel.state.electromag.E, levelNumber, initDataTime);
– Line 780:elecGhostsRefiners_.fill(hybridModel.state.electromag.E, levelNumber, time);• src/amr/level_initializer/hybrid_level_initializer.hpp
–hybMessenger.fillElectricGhosts(E, level, 0.);• src/amr/solvers/solver_ppc.hpp
–fromCoarser.fillElectricGhosts(..., level, newTime);If the
fillsignature has been updated to take aPatchLevel const&, replace alllevelNumber(orlevel.getLevelNumber()) arguments withlevel. Otherwise, leave them as integers.
| } | ||
|
|
||
| template<std::size_t dim> | ||
| requires(!FloatingPoint<value_type>) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify FloatingPoint concept availability.
The code uses a FloatingPoint concept that must be defined elsewhere. Ensure this concept is properly included and available in the current scope.
Also applies to: 79-79
🏁 Script executed:
#!/bin/bash
# Description: Verify FloatingPoint concept is defined and accessible
# Search for FloatingPoint concept definition
rg -A 5 "concept.*FloatingPoint" src/ --type cpp
# Search for includes that might provide FloatingPoint concept
rg "#include.*concept\|#include.*type_traits" src/core/data/grid/ --type cpp
# Check if it's defined in core/def.hpp
rg "FloatingPoint" src/core/def.hppLength of output: 573
Include the FloatingPoint concept’s header in grid.hpp
The FloatingPoint concept is currently defined in src/core/data/ndarray/ndarray_vector.hpp but isn’t brought into scope in src/core/data/grid/grid.hpp. To prevent “undefined identifier” errors around lines 70 and 79, add the appropriate include:
- File: src/core/data/grid/grid.hpp
Insert after the existing#includestatements:#include "../ndarray/ndarray_vector.hpp" // for FloatingPoint concept
Apply the same change before both occurrences of requires(!FloatingPoint<value_type>).
🤖 Prompt for AI Agents
In src/core/data/grid/grid.hpp at line 70, the FloatingPoint concept is used but
its header is not included, causing undefined identifier errors. Fix this by
adding #include "../ndarray/ndarray_vector.hpp" after the existing #include
statements near the top of the file to bring the FloatingPoint concept into
scope. Apply this inclusion before both lines where
requires(!FloatingPoint<value_type>) appears.
- PatchData NaN initialized on construction - fix tests failing as result of above - comment a field refinement test (useless, wrong refinement op for E,B) - debug plots for advance field overlap test - copy done before refinement (boolean false in variable) - overwrite_interior false also for refinement is default for FieldFillPattern - J manually init to zero in model init, fine init and regrid init (Jx unused in ampere but used in Ohm with its now NaN values) - Grid/NdarrayVector take default value overrides (for test) - UsableTensorField is default constructed with zero init.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/simulator/test_advance.py (3)
77-77: Cleanup registration is still disabled - resource leak risk remains.This issue was previously flagged. Consider implementing the suggested configurable approach to allow selective cleanup disabling for debugging while preserving default cleanup behavior.
290-311: Fix ambiguous variable name in makerec function.The variable
lin the list comprehension violates naming conventions and reduces readability, as flagged by static analysis.Apply this fix to improve readability:
def makerec( - lower, upper, dl, fc="none", ec="g", lw=1, ls="-" + lower, upper, spacing, fc="none", ec="g", lw=1, ls="-" ): - origin = (lower[0] * dl[0], lower[1] * dl[1]) + origin = (lower[0] * spacing[0], lower[1] * spacing[1]) sizex, sizey = [ - (u - l) * d for u, l, d in zip(upper, lower, dl) + (u - lower_val) * spacing_val + for u, lower_val, spacing_val in zip(upper, lower, spacing) ]
402-404: Exception handling behavior change already flagged.The commented out error re-throwing and diff_boxes return was previously identified as changing test behavior from fail-fast to log-and-continue. The suggested configurable approach should be implemented.
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
290-385: Consider organizing the extensive debugging code into a helper method.The debugging visualization code is valuable but makes the main test method quite lengthy. Consider extracting this into a separate helper method for better maintainability.
+ def _generate_overlap_failure_diagnostics(self, pd1, pd2, box, ilvl, slice1, slice2, failed_i, failed_j, loc_b1, loc_b2, datahier): + """Generate detailed plots and diagnostics for overlap assertion failures.""" + import matplotlib.pyplot as plt + from matplotlib.patches import Rectangle + + def makerec(lower, upper, spacing, fc="none", ec="g", lw=1, ls="-"): + # ... (move the makerec function here) + + # ... (move all the plotting code here) + try: assert_fp_any_all_close(slice1, slice2, atol=5.5e-15, rtol=0) checks += 1 except AssertionError as e: - # ... (replace with call to helper method) + if box.ndim == 2: + failed_i, failed_j = np.where(np.abs(slice1 - slice2) > 5.5e-15) + self._generate_overlap_failure_diagnostics(pd1, pd2, box, ilvl, slice1, slice2, failed_i, failed_j, loc_b1, loc_b2, datahier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/amr/data/field/field_data.hpp(1 hunks)src/amr/data/field/field_geometry.hpp(1 hunks)src/amr/data/field/field_variable.hpp(1 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(2 hunks)src/amr/data/field/refine/electric_field_refiner.hpp(8 hunks)src/amr/data/field/refine/field_refine_operator.hpp(1 hunks)src/amr/data/field/refine/field_refiner.hpp(3 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(7 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/physical_models/hybrid_model.hpp(2 hunks)src/amr/solvers/solver_ppc.hpp(6 hunks)src/core/data/grid/grid.hpp(3 hunks)src/core/data/ndarray/ndarray_vector.hpp(3 hunks)tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/core/data/ndarray/test_main.cpp(6 hunks)tests/core/data/tensorfield/test_tensorfield_fixtures.hpp(2 hunks)tests/core/numerics/interpolator/test_main.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(2 hunks)tests/simulator/advance/test_fields_advance_2d.py(2 hunks)tests/simulator/test_advance.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/amr/data/field/refine/field_refine_operator.hpp
- src/amr/data/field/field_data.hpp
- tests/amr/data/field/refine/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (20)
- src/amr/data/field/field_variable.hpp
- tests/simulator/advance/test_fields_advance_2d.py
- tests/simulator/advance/test_fields_advance_1d.py
- src/amr/data/field/field_variable_fill_pattern.hpp
- src/amr/data/field/refine/field_refiner.hpp
- src/amr/data/field/field_geometry.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
- src/amr/solvers/solver_ppc.hpp
- tests/core/numerics/interpolator/test_main.cpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- tests/core/data/tensorfield/test_tensorfield_fixtures.hpp
- src/core/data/grid/grid.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/data/field/refine/electric_field_refiner.hpp
- tests/core/data/ndarray/test_main.cpp
- src/amr/physical_models/hybrid_model.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/core/data/ndarray/ndarray_vector.hpp
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/simulator/test_advance.py
290-290: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
306-306: Ambiguous variable name: l
(E741)
| for (auto& patch : *level) | ||
| { | ||
| auto _ = resourcesManager_->setOnPatch(*patch, hybridModel.state.J); | ||
| hybridModel.state.J.zero(); | ||
| } |
There was a problem hiding this comment.
for(auto& /*patch*/: resourcesManager_->enumerate(*level, hybridModel.state.J)
hybridModel.state.J.zero();
?
| auto qty = field.physicalQuantity(); | ||
| using qty_t = decltype(qty); |
There was a problem hiding this comment.
auto const qty = field.physicalQuantity();
using qty_t = std::decay_t<decltype(qty)>;
?
|
replaced by #1068 |
2 participants
See llnl/SAMRAI#292 (comment)
This PR changes the order of copy and refine operations during schedules. Using fine boundary represent variable boolean set to false in the Variable.
With this PR copies are done before refinement. This is tricky and leaves issues for the refinement operator not to overwrite now previously done copy. These issues are addressed in this PR by setting nans.
Consequently, the PR updates the
RefineOperatorso that it assigns values only to the nodes that have NaN values.This thus sets the NaN on construction of
NdArrayVectorand on thePatchDataghost box before ghost schedules apply.Setting NaNs on construction breaks some tests which are fixed so that fields and vecfields they use are properly initialized to 0.
We also see in this PR that in 1D Jx is not used by Ampere (rightfully) and thus used by Ohm (in JxB) with its "cosntruction-initialization value". Now being constructed-init to NaN, J is explicitly set to 0 on init (model init, fine level init and regrid init).
The PR also adds nice plots for debugging when advance test field overlap equal fails.
Blue are overlapped PatchData ghost boxes, red box is the overlap box (pyphare one), red markers indicate lower left corner of the cell that fails
Note: This PR should probably not being merged as is since master currently have multiple components B and E registered to the same RefineAlgorithm and thus SAMRAI uses the first geometry, which now leaves NaNs (rather than wrong 0s) on the mesh. This should be fixed with TensorFieldData.