try fallback for dl on keyerror + ruffage #916
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files. In Changes
Possibly related PRs
Suggested reviewers
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return root_cell_width / fac | ||
|
|
||
| except KeyError: | ||
| ... # time may not be avilaable for given quantity |
Check notice
Code scanning / CodeQL
Statement has no effect Note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
tests/functional/tdtagged/td1dtagged.py (2)
Line range hint
266-273: Consider refactoring global state usage.The function relies on the global
particle_diagnosticsdictionary which could make testing and maintenance more difficult. Consider:
- Passing the diagnostics state as a parameter
- Using a class to encapsulate this state
Example refactor:
-def post_advance(new_time): +def post_advance(new_time, diagnostics): if ( - particle_diagnostics["idx"] < particle_diagnostics["count"] + diagnostics["idx"] < diagnostics["count"] and cpp.mpi_rank() == 0 ): - particle_diagnostics["idx"] += 1 + diagnostics["idx"] += 1
Line range hint
275-284: Add error handling for simulation runs.The main function could benefit from proper error handling to ensure failures are properly reported and resources are cleaned up.
def main(): - Simulator(noRefinement(diagdir="noRefinement")).run().reset() - ph.global_vars.sim = None + try: + Simulator(noRefinement(diagdir="noRefinement")).run().reset() + except Exception as e: + print(f"Error in noRefinement simulation: {e}") + raise + finally: + ph.global_vars.sim = None - Simulator( - withTagging(diagdir="withTagging"), post_advance=post_advance - ).run().reset() - ph.global_vars.sim = None + try: + Simulator( + withTagging(diagdir="withTagging"), post_advance=post_advance + ).run().reset() + except Exception as e: + print(f"Error in withTagging simulation: {e}") + raise + finally: + ph.global_vars.sim = None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- pyphare/pyphare/pharesee/run/run.py (7 hunks)
- pyphare/pyphare/simulator/simulator.py (2 hunks)
- tests/functional/tdtagged/CMakeLists.txt (1 hunks)
- tests/functional/tdtagged/td1dtagged.py (1 hunks)
🧰 Additional context used
🪛 Ruff
pyphare/pyphare/simulator/simulator.py
116-116:
excepthandlers should only be exception classes or tuples of exception classes(B030)
140-140:
excepthandlers should only be exception classes or tuples of exception classes(B030)
🔇 Additional comments (4)
tests/functional/tdtagged/CMakeLists.txt (1)
13-13: LGTM! Fixed CMake function call syntax.The removal of the hyphen corrects the function name syntax, ensuring proper recognition by CMake.
Let's verify the CMake function definition:
✅ Verification successful
Let me check the actual function definition to ensure it accepts the arguments correctly.
Function signature and usage are correct
The
phare_python3_execfunction is properly defined inres/cmake/def.cmakeand accepts exactly 4 arguments (level, target, file, directory) which match the usage in the reviewed code. The function has two implementations based on MPI testing, but both variants support the argument pattern used in the CMakeLists.txt. The example usage comment in the definition also confirms this is the intended syntax.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the phare_python3_exec function exists in CMake files # Expected: Find the function definition in CMake configuration rg -l 'function.*phare_python3_exec'Length of output: 58
Script:
#!/bin/bash # Get the function definition with context rg -A 10 'function.*phare_python3_exec' res/cmake/def.cmakeLength of output: 869
tests/functional/tdtagged/td1dtagged.py (1)
13-17: Verify test base class usage and cpp initialization.Let's verify the integration of these changes with the rest of the codebase.
✅ Verification successful
Let me analyze the results and generate additional verification to understand the inheritance patterns and potential circular dependencies better.
Based on the comprehensive analysis of the results, I can now provide a final response:
The imports and cpp initialization are correctly integrated
AdvanceTestBaseis properly used as a test base class, with multiple test files inheriting from it across the codebase- The
cpp = cpp_lib()initialization follows a consistent pattern used in many test files- The imports and usage in
td1dtagged.pyalign with the established patterns in the codebase- The cpp initialization is used appropriately for MPI rank checking in the
post_advancefunction🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other uses of AdvanceTestBase and cpp initialization patterns echo "Checking AdvanceTestBase usage patterns..." rg -l "AdvanceTestBase" --type py echo "Checking cpp initialization patterns..." rg -l "cpp = cpp_lib\(\)" --type py echo "Checking for potential circular imports..." rg -l "from tests.simulator.test_advance import" --type pyLength of output: 1845
Script:
#!/bin/bash # Check the inheritance hierarchy and implementation details echo "Checking AdvanceTestBase implementation..." rg -A 10 "class AdvanceTestBase" tests/simulator/test_advance.py echo "Checking td1dtagged.py implementation..." cat tests/functional/tdtagged/td1dtagged.py echo "Checking for cpp_lib usage patterns..." rg -A 2 "cpp = cpp_lib\(\)" tests/functional/tdtagged/td1dtagged.pyLength of output: 7305
pyphare/pyphare/pharesee/run/run.py (2)
93-93: LGTM! Boolean condition simplification.Good cleanup of redundant boolean comparisons in
GetB,GetE, andGetJmethods. The code is now more concise while maintaining the same logic.Also applies to: 103-103, 165-165
44-44: Verify the impact of passing None instead of empty list.The change in behavior from passing an empty list to passing
Nonefor thetimesparameter could affect howhierarchy_fromprocesses the request. Please ensure this change is compatible with all callers.Also applies to: 55-55
✅ Verification successful
Let me gather more information about the
hierarchy_fromfunction and its usage to better understand the impact.
Let me check the implementation of
hierarchy_fromh5to understand how it handles thetimeparameter, since that's where thetimesparameter is ultimately used.
Let me check the helper functions that determine the behavior based on the time parameter.
The change from empty list to None is correct and safe
The change in behavior from passing an empty list to passing
Nonefor thetimesparameter is actually correct and safe. Looking at the implementation ofhierarchy_fromh5and its helper functions:
- When
timesisNone, it triggerscreate_from_all_timeswhich loads all available times from the H5 file- When
timesis an empty list, it would triggercreate_from_timesbut still process no times- The change makes the intent clearer by using
Noneto explicitly indicate "load all times" vs. an empty list which suggests "load no times"This aligns better with the function's design and makes the behavior more explicit and intentional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to hierarchy_from to verify time parameter handling ast-grep --pattern 'hierarchy_from($$$, times=$_)'Length of output: 184
Script:
#!/bin/bash # Search for hierarchy_from function definition and usage ast-grep --pattern 'hierarchy_from($$$)' # Also search for the function definition using ripgrep for better context rg "def hierarchy_from" -A 5Length of output: 8887
Script:
#!/bin/bash # Search for the content of hierarchy_fromh5 function to see how it handles time parameter rg "def hierarchy_fromh5" -A 20 pyphare/pyphare/pharesee/hierarchy/fromh5.pyLength of output: 871
Script:
#!/bin/bash # Search for the helper functions that determine the behavior rg "def create_from_times|def create_from_all_times|def load_one_time|def load_all_times" -B 1 -A 5 pyphare/pyphare/pharesee/hierarchy/fromh5.pyLength of output: 539
PhilipDeegan
force-pushed
the
rufftdtagging
branch
from
62bf35d to
89565b6
Compare
PhilipDeegan
force-pushed
the
rufftdtagging
branch
from
89565b6 to
6872684
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pyphare/pyphare/pharesee/run/run.py (1)
Line range hint
236-273: Enhance method documentation and type hints.Consider adding:
- Type hints for parameters and return value
- More descriptive docstring including the return type and possible exceptions
Apply these improvements:
- def GetDl(self, level="finest", time=None): + def GetDl(self, level: Union[str, int] = "finest", time: Optional[float] = None) -> np.ndarray: """ gives the ndarray containing the grid sizes at the given time for the hierarchy defined in the given run, and for the given level (default is 'finest', but can also be a int) :param level: the level at which get the associated grid size :param time: the time because level depends on it + :return: numpy array containing the grid sizes + :raises RuntimeError: if unable to get cell width for the given time + :raises KeyError: if time is not available for a given quantity """Don't forget to add the imports at the top:
from typing import Union, Optional🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 271-271: Statement has no effect
This statement has no effect.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- pyphare/pyphare/pharesee/run/run.py (5 hunks)
- pyphare/pyphare/simulator/simulator.py (2 hunks)
- tests/functional/tdtagged/CMakeLists.txt (1 hunks)
- tests/functional/tdtagged/td1dtagged.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pyphare/pyphare/simulator/simulator.py
- tests/functional/tdtagged/CMakeLists.txt
- tests/functional/tdtagged/td1dtagged.py
🧰 Additional context used
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/run/run.py
[notice] 271-271: Statement has no effect
This statement has no effect.
🔇 Additional comments (1)
pyphare/pyphare/pharesee/run/run.py (1)
93-93: LGTM! Good simplification of boolean conditions.The removal of redundant
== Truecomparisons makes the code more concise and readable while maintaining the same functionality.Also applies to: 103-103, 165-165
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
…putation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem
updated + tested mhd_state, updated mhd_model revert back my typo on hybrid_tagger fixed typo in test_mhd_state preparing mhd usablevecfield new field initialisation logic Added end of file newline in field_user_initializer.hpp added mhd state fixture wrote test for mhd_state_fixtures added mhd yee grid, modified gridlayout to handle both hybrid and mhd up init_functions MHD solver solver mhd update more solver updates preparing rebase formatting riemann reconstruction quick commit fix solved conflict more solver updates initial branch commit: new architecture for reconstruction + flux computation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem added time integration added projection functions for centering, primitive/conservetive converter added projection functions for centering, primitive/conservetive converter solver ref binding in a lambda issue added template deduction guide on for macOS compile on a struct of MHDSolver New approach for template deduction for the struct in MHDSolver added usable ghost cells syntaxe fixes for Werror full boundary conditions in solver changed ghost cells function names to be more consistant with hybrid corrected dummyhierarchy in testmhdsolver (operation on nullptr) MHDMock simulator setup added pybind wrappers and first tests successful orszag-tang test fixed typo fixed typo fixed typo bug fixes (wrong usage of prevIndex nextIndex)
updated + tested mhd_state, updated mhd_model revert back my typo on hybrid_tagger fixed typo in test_mhd_state preparing mhd usablevecfield new field initialisation logic Added end of file newline in field_user_initializer.hpp added mhd state fixture wrote test for mhd_state_fixtures added mhd yee grid, modified gridlayout to handle both hybrid and mhd up init_functions MHD solver solver mhd update more solver updates preparing rebase formatting riemann reconstruction quick commit fix solved conflict more solver updates initial branch commit: new architecture for reconstruction + flux computation improved test, fixed some bugs in godunov-fluxes better timing directory creation (PHAREHUB#914) safer nu (PHAREHUB#911) try fallback for dl on keyerror + ruffage (PHAREHUB#916) convenient utils for mpi/hierarchies keep pyattrs on compute_from_hier nan min/max to handle possible nan ghosts (PHAREHUB#923) * nan min/max to handle possible nan ghosts * flatten rm atefact file fixed missing template keyword for macos-12 build more explicit unwrapping in godunov fluxes with variadic arguments of unclear size fixed lambda capture problem fixed lambda capture problem added time integration added projection functions for centering, primitive/conservetive converter added projection functions for centering, primitive/conservetive converter solver ref binding in a lambda issue added template deduction guide on for macOS compile on a struct of MHDSolver New approach for template deduction for the struct in MHDSolver added usable ghost cells syntaxe fixes for Werror full boundary conditions in solver changed ghost cells function names to be more consistant with hybrid corrected dummyhierarchy in testmhdsolver (operation on nullptr) MHDMock simulator setup added pybind wrappers and first tests successful orszag-tang test fixed typo fixed typo fixed typo bug fixes (wrong usage of prevIndex nextIndex)
Summary by CodeRabbit
New Features
Bug Fixes