better explain field level ghost time interp test#947
better explain field level ghost time interp test#947nicolasaunai merged 1 commit intoPHAREHUB:masterfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request updates several test modules. In one file, the skip message for a test method is modified with a slight wording change. In two refinement test files, the calls to the ghost field verification function have been commented out with added warnings referencing another test file. Additionally, a new method for testing ghost field interpolation during subcycles is introduced along with detailed documentation and debugging output. No changes were made to declarations of exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as Test Runner
participant ATB as AdvanceTestBase
Note over TR,ATB: Ghost Field Verification Test Flow
TR->>ATB: Initiate ghost field verification test
ATB->>ATB: Call base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation()
ATB->>ATB: Validate ghost data via space/time interpolation
TR-->>ATB: Receive test results and assertions
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✨ Finishing Touches
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
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
597-597: Consider using a more descriptive debug message.The debug print statement uses a non-descriptive message "ZOB". Consider using a more meaningful message that indicates what is being debugged.
- print("ZOB", qty) + print(f"Checking field level ghost for quantity: {qty}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/simulator/advance/test_fields_advance_2d.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/refinement/test_2d_2_core.py(1 hunks)tests/simulator/test_advance.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/simulator/advance/test_fields_advance_2d.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
🔇 Additional comments (5)
tests/simulator/refinement/test_2d_2_core.py (1)
168-169: LGTM! Test correctly disabled.The test for field level ghosts via subcycles and coarser interpolation has been correctly disabled, as E and B fields are no longer time refined according to the warning in
test_advance.py.tests/simulator/refinement/test_2d_10_core.py (1)
171-176: LGTM! Test correctly disabled.The test for field level ghosts via subcycles and coarser interpolation has been correctly disabled, as E and B fields are no longer time refined according to the warning in
test_advance.py.tests/simulator/test_advance.py (3)
543-546: LGTM! Clear docstring added.The docstring clearly explains that this method has been extracted from
_test_field_level_ghosts_via_subcycles_and_coarser_interpolationand is used in multiple test files.
696-724: LGTM! Comprehensive docstring update.The docstring has been significantly improved with:
- Clear explanation of the test's purpose
- Detailed requirements and methodology
- Important warning about E and B fields no longer being time refined
740-740: LGTM! Test correctly updated.The test has been correctly updated to test only moments (N, Vi, and J) as they are the only quantities that are space/time interpolated now.
tests/simulator/test_advance.py
Outdated
|
|
||
| This requires: | ||
| - to dump diagnostics at substeps | ||
| - refine spatialy and temporally L0 values and compare them to L1 level ghost values |
nicolasaunai
force-pushed
the
testfix
branch
from
d5475a7 to
78793f7
Compare
2 participants
Summary by CodeRabbit