nd restart tests #809
nd restart tests #809
Conversation
WalkthroughThe changes encompass updates to CMake configurations, library dependencies, and test suite enhancements. Noteworthy modifications include the introduction of a variable for customizing Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (4)
Files skipped from review due to trivial changes (1)
Additional Context UsedRuff (15)
Learnings (1)
Additional comments not posted (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- res/cmake/def.cmake (3 hunks)
- tests/simulator/init.py (3 hunks)
- tests/simulator/test_restarts.py (10 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/simulator/test_tagging.py.off
Additional comments: 14
tests/simulator/__init__.py (5)
- 3-3: Ensure the
deepcopyimport is used within the file to justify its addition.- 36-36: The use of
deepcopyforkwargsinbasicSimulatorArgsis good for preventing side effects on the original argument object.- 52-52: The calculation for
dlas[1.0 / v for v in cells]assumes uniform cell size across dimensions. Ensure this assumption is valid for all use cases.- 58-58: Using
deepcopyforkwargsbefore theupdateoperation ensures that the originalkwargsobject is not modified, which is a good practice for avoiding side effects.- 160-160: Instantiating
ElectronModeldirectly without assigning it or using it in a broader context might indicate an incomplete implementation or a misunderstanding of how models are used within simulations. Verify its purpose and integration.res/cmake/def.cmake (3)
- 14-16: The introduction of
PHARE_MPIRUN_POSTFIXwith a default value--bind-to noneis a good practice for making test execution behavior configurable. Ensure that this new variable is documented and that its impact on MPI behavior is clearly understood by users.- 188-198: The usage of
PHARE_MPIRUN_POSTFIXinadd_phare_test,add_python3_test, andadd_mpi_python3_testfunctions ensures that the new variable is applied consistently across different types of tests. This is a good practice for maintaining consistency in test execution behavior.- 261-261: The use of
PHARE_MPIRUN_POSTFIXinphare_mpi_python3_execfunction extends the configurability to Python-based MPI tests, aligning with the changes made for C++ tests. This consistency is beneficial for maintaining a uniform testing environment.tests/simulator/test_restarts.py (6)
- 20-20: The addition of
populate_simulationimport is necessary for the refactored simulation setup logic. Ensure that this import is used appropriately within the file.- 27-29: The update to
permutefunction to usesupported_dimensionsfrompyphare.pharein.simulationis a good practice for ensuring compatibility with the supported simulation dimensions. This change enhances the flexibility and maintainability of the test setup.- 54-91: The refactoring of
setup_modelfunction and related model setup functions to accept*xyzarguments improves the flexibility and readability of the simulation setup. This change allows for more dynamic model configurations based on simulation dimensions.- 135-148: The introduction of
ModelFunctorandDiagsFunctorclasses for organizing model setup and diagnostics setup logic is a good practice for improving modularity and readability of the test code. Ensure that these functors are used consistently across test cases.- 277-293: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [257-288]
The refactoring of the
test_restartsmethod to handle simulation setup differently, including the use ofpopulate_simulationwith updated arguments, enhances the test's clarity and maintainability. Verify that the new setup logic correctly reflects the intended test scenarios.
- 351-369: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [315-384]
The updates to
test_restarts_elapsed_time,test_mode_conserve, andtest_input_validation_trailing_slashmethods for simulation setup changes improve the specificity and clarity of each test case. Ensure that these changes accurately reflect the intended testing scenarios and that all necessary configurations are correctly applied.
| if "refinement" not in kwargs and "refinement_boxes" not in kwargs: | ||
| b0 = [[3] * dim, [8] * dim] | ||
| args["refinement_boxes"] = {"L0": {"B0": b0}} |
There was a problem hiding this comment.
The condition to set refinement_boxes if not present introduces a hardcoded refinement box. This might not be suitable for all simulations. Consider making refinement boxes configurable or clearly document this behavior.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
157bab5 to
3b457ee
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- res/cmake/def.cmake (3 hunks)
- tests/simulator/init.py (3 hunks)
- tests/simulator/test_restarts.py (8 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- res/cmake/def.cmake
- tests/simulator/init.py
- tests/simulator/test_tagging.py.off
Additional comments: 10
tests/simulator/test_restarts.py (10)
- 20-20: Import of
populate_simulationis added. Ensure this new function is used appropriately and aligns with the PR objectives.- 27-29: The
permutefunction now usessupported_dimensionsfrompyphare.pharein.simulation. Verify thatsupported_dimensionsprovides the expected dimensions for all test cases.- 39-51: The
simArgsdictionary has been updated with new simulation arguments. Ensure that all keys and values are correctly used in the simulation setup and that there are no typos or logical errors in the values assigned.- 54-91: Refactoring of
setup_modelfunction to accept*xyzarguments and updates to the model setup logic. Confirm that the new arguments and the refactored logic correctly configure the simulation models as intended.- 135-148: Reorganization of the
ModelFunctorandDiagsFunctorclasses. Ensure that these changes improve modularity and maintainability without introducing any logical errors or inconsistencies.- 257-257: Conditional logic for setting
refinement_boxes. Ensure that the conditions and the logic for settingrefinement_boxesare correct and align with the intended simulation setup.- 280-280: Setting of
restart_options["timestamps"]directly. Confirm that this direct manipulation of the dictionary is intended and that the value set aligns with the simulation requirements.- 315-318: The
test_restarts_elapsed_timemethod has been refactored to use the new simulation setup. Verify that the changes correctly implement the intended test logic and that the elapsed time calculations are accurate.- 392-394: The
test_mode_conservemethod adjusted for simulation setup changes. Ensure that the adjustments correctly implement the conservation mode testing and that the logic aligns with the simulation requirements.- 419-421: The
test_input_validation_trailing_slashmethod updated for simulation setup changes. Confirm that the addition of extra slashes to the directory path is handled correctly in the simulation setup and does not introduce any issues.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
a5b43b7 to
2c3bc9a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- res/cmake/dep/highfive.cmake (1 hunks)
- tests/simulator/test_restarts.py (8 hunks)
Additional comments: 13
res/cmake/dep/highfive.cmake (2)
- 9-10: Ensure the branch
v2.9.0of HighFive is the intended version for stability and compatibility with the project. Cloning a specific version is a good practice for reproducible builds.- 15-16: Removal of commented-out code for updating HighFive is a good cleanup practice, assuming dynamic updates are no longer required or handled differently.
tests/simulator/test_restarts.py (11)
- 20-20: Importing
populate_simulationdirectly supports test setup modularity and reusability.- 28-28: The commented-out import for
supported_dimensionsis removed, aligning with the summary. Ensure future support for multiple dimensions is tracked if still intended.- 39-51: Refining
simArgswith specific simulation parameters and options directly in the test file enhances test clarity and control over simulation setup.- 54-91: The
setup_modelfunction and its nested functions for defining simulation parameters are well-organized, promoting readability and maintainability. Ensure theppcparameter's usage aligns with test design intentions.- 135-141: The
ModelFunctorclass provides a clear, encapsulated way to configure the model setup for simulations, enhancing test structure.- 143-148: The
DiagsFunctorclass correctly uses its instance variabletimestamps, improving clarity and reducing the risk of bugs related to global variable usage.- 181-181: Consider removing or replacing
- 257-257: The comment block provides valuable context for the setup of
refinement_boxesandtime_step_nbr, aiding in understanding the test's intent.- 280-282: Refactoring the repeated manipulation of
simputdictionary for restart and diagnostic options into a helper function could improve code maintainability.- 365-365: Using
time.sleep(5)in tests could lead to flaky behavior. Consider a more reliable synchronization mechanism if waiting for a condition.- 420-420: Appending slashes to directory paths using string concatenation can lead to errors. Using
os.path.joinorpathlibfor path manipulations ensures cross-platform compatibility.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
2c3bc9a to
04d6f73
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- res/cmake/def.cmake (3 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- tests/simulator/init.py (3 hunks)
- tests/simulator/test_restarts.py (10 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- res/cmake/def.cmake
- res/cmake/dep/highfive.cmake
- tests/simulator/init.py
- tests/simulator/test_tagging.py.off
Additional comments: 11
tests/simulator/test_restarts.py (11)
- 17-17: Consider using relative imports for internal modules to enhance modularity and maintainability.
- 22-24: The import of
supported_dimensionsis now used within thepermutefunction. Ensure this change aligns with the intended use cases and thatsupported_dimensionsprovides the expected values for dimensions.- 34-46: The
simArgsdictionary is defined at the module level, which could lead to unintended side effects if modified by any test. Consider moving it inside a function or using a function to generate a fresh copy for each test to ensure isolation.- 49-87: The model setup functions (
density,bx,by,bz,T,vx,vy,vz,vxalpha,vthxyz) are defined withinsetup_modelbut do not utilize theppcparameter directly. Ensure that the design intention is clear, and consider refactoring if these functions are meant to be more closely integrated with the model setup logic.- 130-136: The
ModelFunctorclass is effectively a wrapper around thesetup_modelfunction. Ensure that this level of indirection is necessary for the test design, as it could potentially be simplified.- 138-143: The
DiagsFunctorclass is initialized withtimestampsbut uses a globaltimestampsvariable in its__call__method. This could lead to confusion or bugs if the global variable changes. Ensure that the instance variable is used as intended.- 176-176: Usage of
- 252-252: The comment block explains the rationale behind setting
refinement_boxesandtime_step_nbr. Ensure that such explanations are maintained up-to-date with the code logic to avoid confusion.- 275-277: The manipulation of
simput["restart_options"]["timestamps"]andsimput["diag_options"]["options"]["dir"]is repeated in multiple test cases. Consider refactoring this logic into a helper function to reduce duplication and improve maintainability.- 360-360: The call to
time.sleep(5)in a test case could lead to flaky tests, especially in different execution environments. If this is intended to wait for some condition, consider using a more reliable synchronization mechanism.- 415-415: Appending slashes to a directory path using string concatenation can lead to errors or unexpected behavior. Use
os.path.joinorpathlibfor path manipulations to ensure cross-platform compatibility.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
04d6f73 to
0935b00
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- res/cmake/def.cmake (3 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- tests/simulator/init.py (3 hunks)
- tests/simulator/test_restarts.py (10 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- res/cmake/def.cmake
- res/cmake/dep/highfive.cmake
- tests/simulator/init.py
- tests/simulator/test_tagging.py.off
Additional comments: 11
tests/simulator/test_restarts.py (11)
- 17-17: Consider using relative imports for internal modules to enhance modularity and maintainability.
- 22-24: The import of
supported_dimensionsis now uncommented, ensuring its functionality is utilized in thepermutefunction. This change aligns with the objective of refining test configurations.- 34-46: The
simArgsdictionary is defined at the module level, which could lead to unintended side effects if modified by any test. Consider moving it inside a function or using a function to generate a fresh copy for each test to ensure isolation.- 49-87: The model setup functions (
density,bx,by,bz,T,vx,vy,vz,vxalpha,vthxyz) are defined withinsetup_modelbut do not utilize theppcparameter. Ensure that the design intention is clear, and consider refactoring if these functions are meant to be more closely integrated with the model setup logic.- 130-136: The
ModelFunctorclass is effectively a wrapper around thesetup_modelfunction. Ensure that this level of indirection is necessary for the test design, as it could potentially be simplified.- 138-143: The
DiagsFunctorclass is initialized withtimestampsbut uses a globaltimestampsvariable in its__call__method. This could lead to confusion or bugs if the global variable changes. Ensure that the instance variable is used as intended.- 176-176: Usage of
- 252-252: The comment block explains the rationale behind setting
refinement_boxesandtime_step_nbr. Ensure that such explanations are maintained up-to-date with the code logic to avoid confusion.- 275-277: The manipulation of
simput["restart_options"]["timestamps"]andsimput["diag_options"]["options"]["dir"]is repeated in multiple test cases. Consider refactoring this logic into a helper function to reduce duplication and improve maintainability.- 358-358: The call to
time.sleep(5)in a test case could lead to flaky tests, especially in different execution environments. If this is intended to wait for some condition, consider using a more reliable synchronization mechanism.- 413-413: Appending slashes to a directory path using string concatenation can lead to errors or unexpected behavior. Use
os.path.joinorpathlibfor path manipulations to ensure cross-platform compatibility.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
0935b00 to
c385c4b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- res/cmake/def.cmake (3 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- tests/simulator/init.py (4 hunks)
- tests/simulator/test_restarts.py (12 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- res/cmake/def.cmake
- res/cmake/dep/highfive.cmake
- tests/simulator/test_tagging.py.off
Additional comments: 18
tests/simulator/__init__.py (7)
- 3-3: Import of
deepcopyadded.- 49-49: Changed
final_timetotime_stepwith a hardcoded value of0.001.- 52-52: Updated
dlkey calculation to be the inverse of cell counts.- 58-59: Logic to update
argsdictionary withkwargsusingdeepcopy.- 60-62: Condition added to set
refinement_boxesif not present.- 147-160: Refactored
populate_simulationfunction with changes in arguments and logic.- 160-160: Replaced
ElectronModelinstantiation withph.ElectronModel.tests/simulator/test_restarts.py (11)
- 17-17: Added
populate_simulationimport.- 22-23: Updated
permutefunction to usesupported_dimensions.- 34-46: Modified
simArgsdictionary with new simulation setup parameters.- 49-86: Refactored model setup functions to accept
*xyzarguments.- 130-136: Reorganized the
ModelFunctorclass.- 138-143: Reorganized the
DiagsFunctorclass.- 175-176: Usage of
- 262-262: Comment block explaining the rationale behind setting
refinement_boxesandtime_step_nbr.- 285-287: Manipulation of
simput["restart_options"]["timestamps"]andsimput["diag_options"]["options"]["dir"]is repeated in multiple test cases.- 368-368: Call to
time.sleep(5)in a test case.- 423-423: Appending slashes to a directory path using string concatenation.
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
c385c4b to
a8730a3
Compare
| np.testing.assert_allclose( | ||
| pd0.dataset[:], | ||
| pd1.dataset[:], | ||
| atol=1e-14, # 1e-15 fails empirically |
There was a problem hiding this comment.
is flakey, could be 1e-12/13?
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 5
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- res/cmake/def.cmake (3 hunks)
- subprojects/cppdict (1 hunks)
- tests/simulator/init.py (4 hunks)
- tests/simulator/test_restarts.py (12 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
Files skipped from review due to trivial changes (2)
- subprojects/cppdict
- tests/simulator/test_tagging.py.off
Additional Context Used
Learnings (1)
tests/simulator/test_restarts.py (2)
User: PhilipDeegan" PR: PHAREHUB/PHARE#784 File: tests/simulator/test_restarts.py:333-339 Timestamp: 2023-11-25T19:21:40.002Z Learning: The user has indicated that minimal changes were made to get the function working, but acknowledges that there may still be redundant code present, such as sleep/wait/advances. The user is open to further streamlining the code.User: PhilipDeegan" PR: PHAREHUB/PHARE#784 File: tests/simulator/test_restarts.py:333-339 Timestamp: 2023-11-25T19:21:58.449Z Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Additional comments not posted (10)
tests/simulator/__init__.py (5)
3-3: Add a comment explaining whydeepcopyis imported and used.
36-36: Ensure thatdeepcopyis necessary for copyingkwargsto avoid unintended side effects.
49-49: Consider making"time_step"configurable to accommodate different simulation needs.
60-62: Ensure that the hardcoded refinement box is suitable for all simulations or make it configurable.
147-160: Verify that all calls topopulate_simulationacross the codebase have been updated to match the new signature.Verification successful
The calls to
populate_simulationintest_restarts.pyhave been updated to match the new signature, as they include the required arguments (ndim,interp,model_fn,diags_fn) and additional keyword arguments unpacked from**simput. This aligns with the updated function signature provided in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to populate_simulation with incorrect signature. ast-grep --lang python --pattern $'populate_simulation($_, $_, $_, $_, $$$)'Length of output: 1925
res/cmake/def.cmake (2)
14-16: Document the purpose ofPHARE_MPIRUN_POSTFIXand its default value in the CMake configuration.
188-188: Ensure that thempiruncommand withPHARE_MPIRUN_POSTFIXis tested across different environments for compatibility.tests/simulator/test_restarts.py (3)
49-86: Clarify the design intention of model setup functions not utilizing theppcparameter and consider refactoring if necessary.
130-136: Evaluate the necessity of theModelFunctorclass as a wrapper aroundsetup_modelfor potential simplification.
229-229: Consider adjusting theatolvalue based on empirical results to avoid flaky tests.
| simArgs = dict( | ||
| # we are saving at timestep 4, and we have seen that restarted simulations with refinement boxes | ||
| # have regridding in places that don't exist in the original simulation | ||
| # we compare the immediate next timestep of both simulations with refinement boxes, as we have seen | ||
| # in this way neither simulations have any regrids, so are still comparable | ||
| time_step_nbr=5, # avoid regrid for refinement boxes https://github.com/LLNL/SAMRAI/issues/199 | ||
| time_step=timestep, | ||
| boundary_types="periodic", | ||
| cells=22, | ||
| dl=0.3, | ||
| diag_options=dict(format="phareh5", options=dict(dir=out, mode="overwrite")), | ||
| restart_options=dict(dir=out, mode="overwrite"), | ||
| ) |
There was a problem hiding this comment.
Move simArgs inside a function or use a factory function to generate a fresh copy for each test to ensure isolation.
| @@ -161,6 +173,7 @@ | |||
|
|
|||
| self.assertGreater(len(timestamps), 0) | |||
| for time in timestamps: | |||
| print(f"checking time {time}") | |||
There was a problem hiding this comment.
Replace debugging print statements with a logging framework if not intended for final code.
- print(f"checking time {time}")
+ # Consider using a logging framework for debug messagesCommittable 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.
| print(f"checking time {time}") | |
| # Consider using a logging framework for debug messages |
PhilipDeegan
force-pushed
the
nd_restarts
branch
from
e39a7dc to
64c39ec
Compare
| @@ -1,7 +1,10 @@ | |||
| import unittest | |||
|
|
|||
| from copy import deepcopy | |||
There was a problem hiding this comment.
Consider separating imports on individual lines for clarity.
- from datetime import datetime
+ import datetimeCommittable 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.
| from copy import deepcopy | |
| from copy import deepcopy | |
| import datetime |
| simput["restart_options"]["timestamps"] = [timestep * restart_idx] | ||
| simput["diag_options"]["options"]["dir"] = local_out | ||
| ph.global_vars.sim = None | ||
| ph.global_vars.sim = ph.Simulation(**simput) | ||
| assert "restart_time" not in ph.global_vars.sim.restart_options | ||
| model = setup_model() | ||
| dump_all_diags(model.populations, timestamps=np.array(timestamps)) | ||
| Simulator(ph.global_vars.sim).run().reset() | ||
| sim = populate_simulation( |
There was a problem hiding this comment.
Refactor repeated logic manipulating simput["restart_options"]["timestamps"] and simput["diag_options"]["options"]["dir"] into a helper function to improve code maintainability.
|
|
||
| from copy import deepcopy | ||
|
|
||
|
|
There was a problem hiding this comment.
Multiple imports on one line.
- from datetime import datetime
+ import datetimeCommittable 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.
| args = { | ||
| "interp_order": interp, | ||
| "smallest_patch_size": smallest_patch_size, | ||
| "largest_patch_size": [20] * dim, | ||
| "time_step_nbr": 1000, | ||
| "final_time": 1.0, | ||
| "time_step": 0.001, |
There was a problem hiding this comment.
Consider making time_step configurable to accommodate different simulation needs.
| def populate_simulation(ndim=1, interp=1, model_fn=None, diags_fn=None, **simInput): | ||
| ph.global_vars.sim = None | ||
| simulation = ph.Simulation(**basicSimulatorArgs(dim, interp, **input)) | ||
| simulation = ph.Simulation(**basicSimulatorArgs(ndim, interp, **simInput)) | ||
| extra_pops = {} | ||
| if "populations" in input: | ||
| for pop, vals in input["populations"].items(): | ||
| if "populations" in simInput: | ||
| for pop, vals in simInput["populations"].items(): | ||
| extra_pops[pop] = defaultPopulationSettings() | ||
| extra_pops[pop].update(vals) | ||
|
|
||
| model = makeBasicModel(extra_pops) | ||
| if "diags_fn" in input: | ||
| input["diags_fn"](model) | ||
| model = model_fn() if model_fn else makeBasicModel(extra_pops) | ||
| if diags_fn: | ||
| diags_fn(model) | ||
|
|
||
| ElectronModel(closure="isothermal", Te=0.12) | ||
| ph.ElectronModel(closure="isothermal", Te=0.12) |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has identified that the function populate_simulation is still being called with the old signature in two files, which contradicts the expected update across the codebase. Additionally, the new function signature is correctly used in several instances within test_restarts.py. Here are the specific locations that need attention:
./tests/simulator/test_restarts.py:424: Old signature used../tests/simulator/data_wrangler.py:25: Old signature used.
It is recommended to update these calls to match the new function signature to ensure consistency and functionality throughout the codebase.
Analysis chain
Ensure all calls to populate_simulation match the new signature and logic.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `populate_simulation` match the new signature.
# Test: Search for the old function signature to find omissions. Expect: No matches.
ast-grep --lang python --pattern $'populate_simulation($_, $_)'
# Test: Search for the new function signature to validate usage. Expect: Matches.
ast-grep --lang python --pattern $'populate_simulation($_, $_, $_, $_, $$$)'
Length of output: 2207
| def hierarchy_by_box(self, hier): | ||
| # comparing hierarchies by index isn't always right | ||
| box_hier = {i: {} for i in range(len(hier.levels()))} | ||
| for ilvl, patches in box_hier.items(): | ||
| for patch in hier.level(ilvl).patches: | ||
| patches[str(patch.box)] = patch | ||
| return box_hier |
There was a problem hiding this comment.
Add unit tests for the hierarchy_by_box method to ensure its correctness.
Would you like me to generate unit tests for the hierarchy_by_box method?
There was a problem hiding this comment.
this is used as part of a test. so it is tested.
There was a problem hiding this comment.
@PhilipDeegan, I've verified that the hierarchy_by_box method is indeed invoked in the tests/simulator/test_restarts.py file. Thus, it is being tested as part of the existing tests.
Thank you for the clarification!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for invocations of the `hierarchy_by_box` method within the test files.
rg 'hierarchy_by_box' tests/
Length of output: 183
| from datetime import timedelta | ||
|
|
||
| from pathlib import Path |
There was a problem hiding this comment.
Remove unused import Path from pathlib.
- from pathlib import PathCommittable 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.
| from pathlib import Path |
| def setup_model(ppc): | ||
| def density(*xyz): | ||
| return 1.0 | ||
|
|
||
| def bx(x): | ||
| def bx(*xyz): | ||
| return 0.0 | ||
|
|
||
| def S(x, x0, l): | ||
| return 0.5 * (1 + np.tanh((x - x0) / l)) | ||
|
|
||
| def by(x): | ||
| def by(*xyz): | ||
| L = ph.global_vars.sim.simulation_domain()[0] | ||
| v1, v2 = -1, 1.0 | ||
| return v1 + (v2 - v1) * (S(x, L * 0.25, 1) - S(x, L * 0.75, 1)) | ||
| return v1 + (v2 - v1) * (S(xyz[0], L * 0.25, 1) - S(xyz[0], L * 0.75, 1)) | ||
|
|
||
| def bz(x): | ||
| def bz(*xyz): | ||
| return 0.5 | ||
|
|
||
| def b2(x): | ||
| return bx(x) ** 2 + by(x) ** 2 + bz(x) ** 2 | ||
| def b2(*xyz): | ||
| return bx(xyz[0]) ** 2 + by(xyz[0]) ** 2 + bz(xyz[0]) ** 2 | ||
|
|
||
| def T(x): | ||
| def T(*xyz): | ||
| K = 1 | ||
| return 1 / density(x) * (K - b2(x) * 0.5) | ||
| return 1 / density(xyz[0]) * (K - b2(xyz[0]) * 0.5) | ||
|
|
||
| def vx(x): | ||
| def vx(*xyz): | ||
| return 2.0 | ||
|
|
||
| def vy(x): | ||
| def vy(*xyz): | ||
| return 0.0 | ||
|
|
||
| def vz(x): | ||
| def vz(*xyz): | ||
| return 0.0 | ||
|
|
||
| def vxalpha(x): | ||
| def vxalpha(*xyz): | ||
| return 3.0 | ||
|
|
||
| def vthxyz(x): | ||
| return T(x) | ||
| def vthxyz(*xyz): |
There was a problem hiding this comment.
Ensure that the ppc parameter is utilized appropriately within the setup_model function.
| class ModelFunctor: | ||
| def __init__(self, ppc=None): | ||
| self.nppc = ppc or nppc | ||
|
|
||
| def __call__(self): | ||
| return setup_model(self.nppc) | ||
|
|
There was a problem hiding this comment.
Evaluate the necessity of the ModelFunctor class as it primarily wraps the setup_model function.
| atol=1e-14, # 1e-15 fails empirically | ||
| rtol=0, | ||
| ) |
There was a problem hiding this comment.
Consider making atol configurable to handle potential flakiness in tests.
|
|
||
| # autodump false to ignore possible init dump | ||
| simulator = Simulator(ph.global_vars.sim, auto_dump=False).initialize() | ||
| simulator = Simulator(sim, auto_dump=False).initialize() |
There was a problem hiding this comment.
Replace time.sleep(5) with a more reliable synchronization mechanism to avoid flaky tests.
| sim = populate_simulation(ndim, interp, model_fn=ModelFunctor(), **simput) | ||
| self.assertEqual(len(sim.restart_options["timestamps"]), 1) | ||
| self.assertEqual(sim.restart_options["timestamps"][0], 0.004) | ||
| Simulator(ph.global_vars.sim).run().reset() | ||
|
|
||
| # second simulation (not restarted) | ||
| ph.global_vars.sim = None | ||
| simput["restart_options"]["mode"] = "conserve" | ||
| ph.global_vars.sim = ph.Simulation(**simput) | ||
| self.assertEqual(len(ph.global_vars.sim.restart_options["timestamps"]), 0) | ||
| sim = populate_simulation(ndim, interp, model_fn=ModelFunctor(), **simput) | ||
| self.assertEqual(len(sim.restart_options["timestamps"]), 0) |
There was a problem hiding this comment.
Clarify the handling of timestamps in the conservation mode to ensure clear and optimal functionality.
| @@ -11,6 +11,10 @@ endif() # clang | |||
| set (PHARE_LINK_FLAGS ) | |||
| set (PHARE_BASE_LIBS ) | |||
|
|
|||
| if(NOT DEFINED PHARE_MPIRUN_POSTFIX) | |||
There was a problem hiding this comment.
without this, running multiple instances of mpirun -n 2 python3 ... effectively only lets run one process, the rest are descheduled until the first is finished
we could not default it here but set it manually in TC builds, or have it as an option defaulted to ON
There was a problem hiding this comment.
this was moved to the configurator, so to rm
Summary by CodeRabbit
Chores
v2.9.0) for better compatibility and stability.New Features
PHARE_MPIRUN_POSTFIXin the CMake configuration to customize thempiruncommand for test executions.Tests