compute overlap refac #994
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant CU as compute_overlaps
participant PPS as possible_periodic_shifts
participant Shift as shift() from pyphare.core.box
participant Overlap as Overlap List
CU->>PPS: Call possible_periodic_shifts(patch, domain_box)
PPS->>Shift: Compute shifted box for each dimension
Shift-->>PPS: Return shifted box
PPS-->>CU: Return list of (offset, shifted box)
CU->>Overlap: Append computed overlaps based on shifts
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
pyphare/pyphare/pharesee/geometry.py:216
- The logic appends an overlap twice when the offset is non-zero. If duplicate entries are not intended, consider revising the condition or adding a comment to clarify the rationale.
if offset != zero_offset:
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py:129
- [nitpick] Using a plain assertTrue without an error message can make test failures harder to diagnose. Consider using assertIn or adding a custom error message for clarity.
self.assertTrue(actual in expected[ilvl])
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| (np.asarray(actual["offset"]) == np.asarray(exp["offset"])).all() | ||
| ) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected[ilvl]) |
Check notice
Code scanning / CodeQL
Imprecise assert Note test
| overlap_boxes += [actual["box"]] | ||
| self.assertEqual(len(expected), len(tested_overlaps[ilvl])) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected) |
Check notice
Code scanning / CodeQL
Imprecise assert Note test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pyphare/pyphare/pharesee/geometry.py (1)
187-226: Refactored overlap computation using the new periodic shifts function.The updated
compute_overlapsfunction now leverages thepossible_periodic_shiftsfunction to more systematically calculate overlaps between patches. This makes the code more modular and easier to understand.However, there's a potential issue at lines 216-224: when
offset != zero_offset, the same overlap is added twice to theoverlapslist. This appears to be intentional but could lead to duplicate entries.Consider adding a comment explaining why duplicate entries are intentionally added when
offset != zero_offsetto improve code clarity.pyphare/pyphare_tests/test_pharesee/test_geometry.py (1)
81-95: Improved test structure with better organization of overlaps.The refactored test now uses a dictionary structure to better organize and verify overlaps, which improves readability and makes the test more maintainable.
There's an unused loop variable on line 89. Consider renaming
lvlto_lvlto follow Python conventions for unused variables:-for ilvl, lvl in enumerate(hierarchy.patch_levels): +for ilvl, _lvl in enumerate(hierarchy.patch_levels):🧰 Tools
🪛 Ruff (0.8.2)
89-89: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
117-123: Enhanced test organization with dictionary for overlap validation.Similar to the changes in test_geometry.py, the refactored test now uses a dictionary to better structure and verify overlaps, improving readability and maintainability.
You're using
.keys()unnecessarily on line 122. A more Pythonic approach would be:-for ilvl in expected.keys() +for ilvl in expected🧰 Tools
🪛 Ruff (0.8.2)
122-122: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
124-130: Test assertion now validates overlaps existence properly.The test verification logic has been improved to check each actual overlap against the expected set.
There's an unused loop variable on line 124. Consider renaming
lvlto_lvlto follow Python conventions for unused variables:-for ilvl, lvl in enumerate(hierarchy.levels().items()): +for ilvl, _lvl in enumerate(hierarchy.levels().items()):🧰 Tools
🪛 Ruff (0.8.2)
124-124: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare/pharesee/geometry.py(1 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry.py(2 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
levels(214-217)
🪛 Ruff (0.8.2)
pyphare/pyphare_tests/test_pharesee/test_geometry.py
89-89: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
122-122: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
124-124: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
pyphare/pyphare/pharesee/geometry.py (2)
152-184: Well-structured implementation of periodic shifts calculation.This new function
possible_periodic_shiftsprovides a clean implementation for computing all possible periodic shifts of a box within a domain. It handles 1D, 2D, and 3D cases systematically, which improves the readability and maintainability of the code.
229-229: New placeholder function preserves backwards compatibility.The addition of
compute_overlaps_appears to preserve the original implementation for backwards compatibility while the main function has been refactored. This is a good approach for maintaining compatibility during significant refactoring.pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
170-179: Improved large patch overlaps test structure.The test for large patch overlaps now uses the same pattern as the regular overlaps test, creating consistency in the testing approach across different scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
124-130: Improve assertion logic and fix static analysis issues.There are two issues identified by static analysis:
- The loop variable
lvlis not used in the loop body- Using
assertTrue(a in b)provides less informative error messages thanassertIn(a, b)-for ilvl, lvl in enumerate(hierarchy.levels().items()): +for ilvl, _lvl in enumerate(hierarchy.levels().items()): if ilvl not in expected: continue self.assertEqual(len(expected[ilvl]), len(level_overlaps[ilvl])) - for actual in tested_overlaps[ilvl]: - self.assertTrue(actual in expected[ilvl]) + for actual in tested_overlaps[ilvl]: + self.assertIn(actual, expected[ilvl])🧰 Tools
🪛 Ruff (0.8.2)
124-124: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
170-179: Improve test validation approach in large patch overlaps test.Similar to the above changes, this test has been refactored to use the
tested_overlapsdictionary approach. The same static analysis issue withassertTruevs.assertInexists here.self.assertEqual(len(expected), len(tested_overlaps[ilvl])) for actual in tested_overlaps[ilvl]: - self.assertTrue(actual in expected) + self.assertIn(actual, expected)
🧹 Nitpick comments (2)
pyphare/pyphare_tests/test_pharesee/test_geometry.py (1)
89-95: Improved assertion logic but with possible enhancements.The updated assertion logic checks if each actual overlap exists in the expected overlaps list, which is more flexible than strict order-based comparison. However, there are two potential improvements:
- The unused loop variable should be renamed with underscore prefix
- Consider using
assertIninstead of manually raising an AssertionError for more informative test failures-for ilvl, lvl in enumerate(hierarchy.patch_levels): +for ilvl, _lvl in enumerate(hierarchy.patch_levels): self.assertEqual(len(expected[ilvl]), len(overlaps[ilvl])) for actual in tested_overlaps[ilvl]: - if actual not in expected[ilvl]: - raise AssertionError(f"Unexpected overlap: {actual}") + self.assertIn(actual, expected[ilvl], f"Unexpected overlap: {actual}")🧰 Tools
🪛 Ruff (0.8.2)
89-89: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
117-123: Good restructuring of test validation.The
tested_overlapsdictionary approach brings consistency with the changes in the other test file and simplifies the assertion logic.However, the dictionary key check can be optimized:
- for ilvl in expected.keys(): + for ilvl in expected:🧰 Tools
🪛 Ruff (0.8.2)
122-122: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare/pharesee/geometry.py(2 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry.py(2 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
levels(214-217)
🪛 Ruff (0.8.2)
pyphare/pyphare_tests/test_pharesee/test_geometry.py
89-89: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
122-122: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
124-124: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
pyphare/pyphare/pharesee/geometry.py (2)
152-184: Well-designed implementation of periodic shifts calculator.The new
possible_periodic_shiftsfunction is well-structured and effectively handles different dimensionality cases (1D, 2D, and 3D). The code is clear, consistent, and follows the same pattern across all dimensions.I appreciate how you've used the nested loops to systematically generate all combinations of shifts in each dimension. This approach makes the code more maintainable and easier to understand compared to hardcoding specific shift patterns.
212-248:❓ Verification inconclusive
Good refactoring to improve code clarity at the expense of some performance.
The refactored
compute_overlapsfunction now systematically tests all possible periodic shifts for each patch data ghost box, making the logic more straightforward to understand. This aligns with the PR's stated goal of improving code clarity, even if it's less efficient.A few observations:
- The
zero_offsetinitialization properly handles both 1D and multi-dimensional cases- The systematic iteration through all possible shifts is more maintainable than previous complex manual calculations
- Lines 239-247 appear to be creating duplicate entries when offset is not zero, which seems intentional for symmetric overlaps
Run this script to verify the overlaps are correctly computed and consistent with test expectations:
🏁 Script executed:
#!/bin/bash # Check if the overlap computations are passing in tests cd pyphare/pyphare_tests/test_pharesee python -m unittest test_geometry.py test_geometry_2d.pyLength of output: 1488
Attention: Verify Test Dependencies
The refactoring of the
compute_overlapsfunction clearly improves code clarity and maintainability. Key points are:
- Zero Offset Handling: The initialization correctly handles both 1D and multi-dimensional cases.
- Systematic Iteration: Iterating over all possible periodic shifts simplifies the logic compared to previous manual computations.
- Duplicate Entries for Non-Zero Offsets: The intentional addition of a duplicate overlap entry when the offset is non-zero ensures symmetric overlaps are captured as intended.
However, the test execution failed due to missing modules (
ddtandnumpy), indicating that the test environment lacks some required dependencies rather than pointing to a flaw in the refactored code. Please ensure that these necessary dependencies are installed (for example, via pip or by updating your project’s requirements file) and manually re-run the tests to verify that the overlap computations work as expected.pyphare/pyphare_tests/test_pharesee/test_geometry.py (1)
81-87: Good restructuring of test verification.Creating the
tested_overlapsdictionary simplifies the validation logic by separating the data structure transformation from the actual assertions. This approach makes it easier to focus on the essential content of overlaps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyphare/pyphare/pharesee/geometry.py (1)
239-248: Good handling of symmetrical overlapsWhen the offset is not zero, the code correctly computes the reciprocal overlap with inverted offsets. This ensures that overlaps are properly identified regardless of which patch is considered the reference.
However, there's a potential optimization opportunity here. Since you're already iterating through all patch pairs, you're computing each overlap twice (once in each direction). You could potentially simplify by computing each overlap only once, but it would require changing how results are used downstream.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyphare/pyphare/pharesee/geometry.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (5)
pyphare/pyphare/pharesee/geometry.py (5)
152-185: Well-structured implementation of periodic shifts computationThe
possible_periodic_shiftsfunction provides a systematic approach to computing all possible periodic shifts for a given box within a domain. The implementation handles 1D, 2D, and 3D cases appropriately with nested loops that scale with the dimensionality.This approach is elegant and makes the concept of periodic boundary conditions more explicit and easier to understand than the previous implementation.
212-212: Good handling of dimensionality in zero_offsetThis line correctly initializes the
zero_offsetvariable based on the dimensionality of the domain box, which is important for proper comparison later in the code.
215-216: Simpler loop pattern improves readabilityThe change to iterate through patches from the current index onwards (
patches[ip:]) rather than a more complex loop pattern simplifies the code and makes it easier to understand.
220-222: Clear extraction of ghost boxesExtracting the ghost boxes into clearly named variables improves code readability.
223-238: Systematic approach to computing overlapsThis code segment systematically computes overlaps between patches using periodic shifts. The approach is more thorough and explicit than the previous implementation, making it easier to understand and maintain.
The condition on line 225-227
if overlap is not None and not np.all(overlap.shape == gb_ref.shape)ensures that only partial overlaps are considered, not complete overlaps, which is correct.
This PR suggests a refac of the computation of overlaps in python hierarchies.
The strategy is less efficient but simpler to understand and maintain. We test the overlap of each patchdata with all others in all their possible periodic shift versions.