keep pyattrs on compute_from_hier #919
Conversation
📝 WalkthroughWalkthroughThe changes primarily involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![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
🧹 Outside diff range and nitpick comments (1)
tests/simulator/test_run.py (1)
237-241: Consider more robust verification methods.The current approach of verifying file existence and minimum size might be fragile. Consider:
- Using content-based verification instead of size-based checks
- Implementing proper cleanup of generated files in tearDown
- Adding explicit error messages explaining why a particular size threshold was chosen
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- tests/simulator/test_run.py (2 hunks)
🔇 Additional comments (4)
tests/simulator/test_run.py (2)
223-229: Well-structured test additions addressing the core issue.The new assertions effectively verify that patch attributes are preserved in both
all_primal=Trueandall_primal=Falsecases, directly addressing the issue described in #918.
19-21: Verify the impact of reduced simulation time.The simulation time has been halved from 0.1 to 0.05. While this might improve test execution speed, please verify that this shorter duration is sufficient to capture all relevant physics phenomena and test scenarios.
✅ Verification successful
Reduced simulation time is appropriate for this test case
The codebase analysis shows that different tests use varying time configurations based on their specific testing needs:
- Most unit tests in
tests/simulator/use small time steps (0.001-0.005) and short durations- Only functional tests (e.g., dispersion.py, translation1d.py) use longer durations (20.0-100.0) for physics validation
The change from 0.1 to 0.05 in test_run.py is consistent with other unit tests' time scales and appears sufficient for testing the patch attributes functionality, which is the focus of this PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other tests that might be using similar time configurations rg -l "final_time\s*=\s*0\.1" tests/Length of output: 38
Script:
#!/bin/bash # Let's try a broader search for time configurations in tests rg -l "final_time\s*=" tests/ # Also search for timestamp configurations rg -l "timestamps\s*=" tests/ # Look for any numerical time values in test files rg "time.*=.*0\.[0-9]+" tests/Length of output: 4714
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (2)
195-196: LGTM! More Pythonic iteration style.The change to use enumeration directly over patches improves code readability while maintaining the same functionality.
202-204: LGTM! Preserves patch attributes as intended.This change correctly addresses the issue by ensuring patch attributes are preserved when creating new patches. This fixes the problem described in issue #918 where patch attributes were empty when
all_primal=True.
closes #918
Summary by CodeRabbit
New Features
Bug Fixes
Documentation