Mpi nightly build trials #917
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications across multiple files in the PHARE project, focusing on error handling in diagnostic options, test configuration adjustments, and simulation setup refinements. The changes primarily involve updating error checking in the simulation module, modifying test execution configurations in CMakeLists.txt files, and restructuring the Harris plasma simulation test scripts to improve modularity and reduce potential runtime errors. Changes
Assessment against linked issues
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
🧹 Outside diff range and nitpick comments (1)
pyphare/pyphare/pharein/simulation.py (1)
501-501: The fix looks good but could be more robust.The added condition helps prevent the KeyError when "options" is commented out, which addresses the issue #896. However, we can make the code more robust by combining the conditions.
Consider this more concise and robust approach:
- if "options" in diag_options and "mode" in diag_options["options"]: + if diag_options.get("options", {}).get("mode"):This change:
- Uses
dict.get()with a default empty dict to safely handle missing "options"- Chains another
get()to safely access "mode"- Reduces nesting and improves readability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- pyphare/pyphare/pharein/simulation.py (1 hunks)
- tests/functional/harris/CMakeLists.txt (1 hunks)
- tests/functional/harris/harris_2d.py (0 hunks)
- tests/simulator/refinement/CMakeLists.txt (1 hunks)
💤 Files with no reviewable changes (1)
- tests/functional/harris/harris_2d.py
🔇 Additional comments (4)
tests/functional/harris/CMakeLists.txt (2)
11-13: LGTM: Improved readability with proper spacing.The added empty lines and clear comment about HighFive dependency improve the readability of the CMake configuration.
Also applies to: 17-17
15-15: Verify the impact of reducing MPI processes.The reduction from 10 to 4 MPI processes could affect test coverage and execution characteristics. Please ensure this change:
- Maintains adequate parallel execution testing
- Doesn't compromise the test's ability to detect race conditions
- Aligns with the minimum required processes for the harris_2d test
✅ Verification successful
Let me gather more information about the harris_2d test to understand its MPI requirements better.
Let me check the MPI execution configuration pattern across the codebase to understand if there's a standard requirement.
Reducing MPI processes from 10 to 4 is acceptable for this test
Based on the analysis of the codebase:
- The harris_2d test is a 2D simulation that doesn't have any strict MPI process count requirements in its implementation
- Other similar 2D tests in the codebase run with 3-4 MPI processes (e.g., test_diagnostics.py runs with both 3 and 4 processes)
- The test's grid configuration (200x400 cells) can be effectively parallelized with 4 processes
- The reduction aligns with the general pattern seen in other tests where most MPI tests use between 3-4 processes
The change appears to be part of a broader effort to standardize MPI process counts across tests, making the test suite more resource-efficient while maintaining adequate parallel execution coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any configuration or documentation that might specify process requirements rg -g '!*.pyc' -g '!*.so' "harris_2d" -A 5 -B 5Length of output: 1312
Script:
#!/bin/bash # Look for the actual test file content and any configuration cat tests/functional/harris/harris_2d.py # Search for any MPI-related patterns in the test rg -g '!*.pyc' -g '!*.so' "mpi" -i tests/functional/harris/Length of output: 4950
Script:
#!/bin/bash # Look for other phare_mpi_python3_exec usages to understand process count patterns rg "phare_mpi_python3_exec" -A 1 -B 1 # Check the CMake function definition fd -e cmake phare_mpi_python3_execLength of output: 4022
tests/simulator/refinement/CMakeLists.txt (2)
19-19: Verify test parameters and dependenciesThe new test execution looks good, but we should ensure it's an adequate replacement for the disabled MPI test.
Let's verify the test script exists and check its contents:
#!/bin/bash # Description: Verify test script existence and contents test_file="${PWD}/tests/simulator/refinement/test_2d_2_core.py" if [ -f "$test_file" ]; then echo "Test file exists" # Look for specific test configurations rg -A 5 "def test|class.*Test" "$test_file" else echo "Test file not found" fi
13-16: LGTM! Well-documented test disablingThe comment properly references the GitHub issue (#853) explaining why the MPI test was disabled, which helps maintain code clarity and traceability.
Let's verify if the referenced issue is still open and relevant:
✅ Verification successful
Let me try to gather more information about this issue and the test files to provide a better verification.
Let me try one more verification to check the content of both test files to understand the relationship between them:
Disabled test is properly documented and replaced with a simpler version
Based on the verification results, I can confirm that:
- The disabled test (
test_2d_10_core.py) and its replacement (test_2d_2_core.py) are similar tests that check Yee Lattice centering discrepancies- The main difference is that the disabled test was more complex and required 10 cores, while the new test is simpler and requires only 2 cores
- The issue reference (test_refine_field shape mismatch #853) is properly documented in the CMakeLists.txt file
Since we couldn't verify the GitHub issue status but found that the test has been replaced with a simpler version that tests the same functionality, the change appears to be a valid optimization of the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if issue #853 is still open gh issue view 853 --json state,titleLength of output: 292
Script:
#!/bin/bash # Check if the issue reference exists in git history or comments rg -i "issue.*853|#853" --type txt # Look for related test files fd "test_2d.*\.py" tests/simulator/refinement/ # Check the content of the commented test file rg "test_2d_10_core.py" tests/simulator/refinement/ -A 5Length of output: 1204
Script:
#!/bin/bash # Compare the content and structure of both test files echo "=== test_2d_2_core.py ===" head -n 20 tests/simulator/refinement/test_2d_2_core.py echo -e "\n=== test_2d_10_core.py ===" head -n 20 tests/simulator/refinement/test_2d_10_core.py # Check if these are similar test cases rg "class.*Test" tests/simulator/refinement/test_2d_2_core.py tests/simulator/refinement/test_2d_10_core.pyLength of output: 1284
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/functional/harris/harris_2d_lb.py (1)
18-18: Consider documenting the SCOPE_TIMING change impact.The default value change from
TruetoFalsefor SCOPE_TIMING means timing data won't be collected by default. While this might improve performance, it could affect debugging and performance analysis capabilities.Consider adding a comment explaining:
- Why timing is now opt-in rather than default
- How to enable timing when needed for debugging
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/functional/harris/CMakeLists.txt (1 hunks)
- tests/functional/harris/harris_2d_lb.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/CMakeLists.txt
🔇 Additional comments (1)
tests/functional/harris/harris_2d_lb.py (1)
Line range hint
91-94: Verify numerical stability of temperature calculation.The temperature calculation involves division by density and subtraction operations which could potentially lead to numerical instability:
def T(x, y): K = 0.7 temp = 1.0 / density(x, y) * (K - b2(x, y) * 0.5) assert np.all(temp > 0) return tempLet's verify the numerical stability:
✅ Verification successful
Let me analyze the density implementation and related tests to verify the numerical stability concerns.
Temperature calculation is numerically stable
The density function implementation shows that it's numerically stable and cannot be zero:
- Base density of 0.4 (or 0.2 in some variants)
- Plus two positive hyperbolic secant terms that are always positive
- The hyperbolic secant terms use well-defined parameters (L, fixed positions at 0.3L and 0.7L)
The temperature calculation is also protected:
- Has an assertion
assert np.all(temp > 0)to catch any violations- Uses a constant K (0.7) and b2(x, y) term which is squared (thus non-negative)
- The density denominator is guaranteed to be positive and bounded away from zero
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing test cases that verify temperature bounds rg -l "def test.*temp" tests/ # Search for any existing assertions on density to ensure it's never zero rg "assert.*density" tests/Length of output: 61
Script:
#!/bin/bash # Search for density function implementation to understand its bounds ast-grep --pattern 'def density(x, y): $$$' # Search for any test files related to Harris model fd -e py harris tests/ # Look for any assertions or checks in the Harris model files rg "assert|check" -A 2 -B 2 tests/functional/harris/Length of output: 4649
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/simulator/CMakeLists.txt (1)
31-31: Remove extra whitespace for consistent indentation.There's an extra blank line with whitespace that should be removed to maintain consistent indentation throughout the file.
else() phare_python3_exec(11 test_diagnostic_timestamps test_diagnostic_timestamps.py ${CMAKE_CURRENT_BINARY_DIR}) - endif(testMPI)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tests/functional/harris/CMakeLists.txt(1 hunks)tests/simulator/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/CMakeLists.txt
🔇 Additional comments (1)
tests/simulator/CMakeLists.txt (1)
29-32: Verify the impact of moving test_diagnostic_timestamps execution.
The restructuring to execute test_diagnostic_timestamps.py only when testMPI is false looks logical. However, please verify that this change in test execution timing doesn't impact the test coverage or CI pipeline expectations.
✅ Verification successful
Moving test_diagnostic_timestamps.py to non-MPI execution is safe
After examining the test file and its history, this change is appropriate because:
- The test uses
startMPI()only for setup and doesn't rely on multiple MPI ranks - The test focuses on diagnostic timestamps and file I/O operations, not MPI-specific functionality
- Git history shows no MPI-specific requirements or past issues related to MPI execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if test_diagnostic_timestamps.py has any MPI-specific code or dependencies
# that might be affected by this change
# Search for MPI-related imports or usage
echo "Checking for MPI usage in test_diagnostic_timestamps.py:"
rg -i "mpi|comm|rank|size" test_diagnostic_timestamps.py
# Check if the test was previously being executed in CI logs
echo "Checking previous CI runs for test execution patterns:"
gh run list --limit 10 --json conclusion,name | jq '.[] | select(.name | contains("test-diagnostic-timestamps"))'
Length of output: 486
Script:
#!/bin/bash
# Let's first locate the test file and check its contents
echo "Locating test_diagnostic_timestamps.py:"
fd "test_diagnostic_timestamps.py"
# If found, let's examine its contents and dependencies
if fd "test_diagnostic_timestamps.py" -x cat {};
# Let's also check the CMake configuration for any related test setup
echo -e "\nChecking CMake test configurations:"
rg "test_diagnostic_timestamps" -A 3 -B 3
Length of output: 255
Script:
#!/bin/bash
# Now that we found the file, let's examine its contents
echo "Contents of test_diagnostic_timestamps.py:"
cat tests/simulator/test_diagnostic_timestamps.py
# Let's also check if this test was previously in a different location
echo -e "\nChecking git history for any moves:"
git log --full-history --follow tests/simulator/test_diagnostic_timestamps.py
# Check the broader context of the CMake configuration
echo -e "\nChecking broader CMake context:"
cat tests/simulator/CMakeLists.txt
Length of output: 9753
|
800**2 harris 2 level harris running in <13 hours on TC with this maybe 200**2 is ok for nightly build long term? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/functional/harris/harris_2d_100_x_100.py (1)
20-30: Update documentation to match defaultSCOPE_TIMINGvalue.The
SCOPE_TIMINGvariable defaults toFalse, but the accompanying documentation focuses on how to enable scope timings. Consider updating the documentation to clarify that scope timings are disabled by default and provide instructions on how to enable them if needed.tests/functional/harris/harris_2d.py (2)
14-14: Remove unused variablecpp.The variable
cppis assigned but not used indefault_setup(). Removing this unused variable will clean up the code and prevent confusion.🧰 Tools
🪛 Ruff (0.8.2)
14-14: Local variable
cppis assigned to but never usedRemove assignment to unused variable
cpp(F841)
24-24: Remove unused variabletimestamps.The variable
timestampsis calculated but not utilized indefault_setup(). Consider removing it or implement it if it's intended for future use.🧰 Tools
🪛 Ruff (0.8.2)
24-24: Local variable
timestampsis assigned to but never usedRemove assignment to unused variable
timestamps(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pyphare/pyphare/pharein/simulation.py(1 hunks)tests/functional/harris/CMakeLists.txt(1 hunks)tests/functional/harris/harris_2d.py(3 hunks)tests/functional/harris/harris_2d_100_x_100.py(2 hunks)tests/simulator/CMakeLists.txt(1 hunks)tests/simulator/refinement/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/simulator/CMakeLists.txt
- tests/simulator/refinement/CMakeLists.txt
- tests/functional/harris/CMakeLists.txt
- pyphare/pyphare/pharein/simulation.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/functional/harris/harris_2d.py
14-14: Local variable cpp is assigned to but never used
Remove assignment to unused variable cpp
(F841)
24-24: Local variable timestamps is assigned to but never used
Remove assignment to unused variable timestamps
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
tests/functional/harris/harris_2d_100_x_100.py (5)
16-16: Importingharris_2dasbaseconsolidates configuration.The import statement
import harris_2d as basebrings in the common configuration fromharris_2d.py, promoting code reuse and modularity.
37-38: Confirm adjusted simulation grid size and time steps meet requirements.The
cellsparameter has been changed to(100, 100), andtime_stepsis set to1000. This reduces the simulation resolution and duration. Ensure that these settings still achieve the desired accuracy and that any analysis based on the simulation remains valid.
46-46: Ensure diagnostic directory path is correct and accessible.The
diag_dirhas been updated to"phare_outputs/harris_2d_100_x_100". Verify that this directory will be correctly created and that the application has necessary permissions to write to this location.
57-58: Verify effect of commenting outrefinementparameter.The
refinement="tagging"parameter is commented out, andmax_nbr_levelsis set to1. Confirm that this change disables mesh refinement as intended and does not adversely affect the simulation outcomes.
70-70: Check compatibility withbase.config(sim).By calling
sim = base.config(sim), the existing simulation configuration is extended. Ensure that theconfigfunction inharris_2d.pycorrectly accepts and modifies the passedsimobject.tests/functional/harris/harris_2d.py (1)
46-49: Validate changes to theconfigfunction signature.The
configfunction now accepts an optionalsimparameter and initializes it usingdefault_setup()if not provided. Ensure that all calls toconfigacross the codebase pass the appropriate arguments and that this change does not introduce any unintended side effects.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/functional/harris/harris_2d.py (1)
Line range hint
25-44: Good encapsulation of simulation setup, consider adding docstring.The default_setup function nicely encapsulates the simulation parameters. Consider adding a docstring to document the chosen parameters and their significance.
def default_setup(): + """ + Creates default simulation setup with 200x400 cells and specific parameters. + + Returns: + ph.Simulation: Configured simulation instance + """ return ph.Simulation(tests/functional/harris/harris_2d_100_x_100.py (1)
68-68: Add error handling for base configuration.While using the base configuration improves code reuse, consider adding error handling for potential configuration failures.
- sim = base.config(sim) + try: + sim = base.config(sim) + except Exception as e: + raise RuntimeError(f"Failed to apply base configuration: {e}") from e
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/functional/harris/harris_2d.py(4 hunks)tests/functional/harris/harris_2d_100_x_100.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/functional/harris/harris_2d_100_x_100.py
19-19: Undefined name os
(F821)
31-31: Undefined name os
(F821)
41-41: Undefined name cpp
(F821)
42-42: Undefined name timestamps
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
tests/functional/harris/harris_2d.py (2)
46-48: Good modularization with optional parameters.The addition of optional sim and seed parameters improves reusability while maintaining backward compatibility.
138-138: Good addition of seed parameter for reproducibility.Adding the seed parameter to protons configuration enables reproducible simulations, which is crucial for testing.
tests/functional/harris/harris_2d_100_x_100.py (4)
19-29: Excellent documentation of scope timing requirements.Clear documentation of prerequisites and configuration requirements for scope timing functionality.
🧰 Tools
🪛 Ruff (0.8.2)
19-19: Undefined name
os(F821)
54-56: Verify impact of disabled refinement and reduced levels.The removal of refinement and reduction in max_nbr_levels will improve performance but might affect simulation accuracy. Please verify that this simplified configuration still meets the test requirements.
Consider documenting the trade-offs between performance and accuracy in the test documentation.
15-16:⚠️ Potential issueAdd missing os import.
The os module is used but not imported.
+import os import harris_2d as baseLikely invalid or redundant comment.
36-39: Verify performance impact of reduced grid size.The reduction from 800x800 to 100x100 cells should significantly improve build time, aligning with the PR objective. However, ensure this doesn't compromise the test's effectiveness.
✅ Verification successful
Grid size reduction is appropriate for test purposes
The reduction to 100x100 cells is reasonable as:
- It's a dedicated fast-running test variant, while the base Harris test retains a higher resolution (200x400)
- The configuration maintains sufficient resolution for testing purposes
- The change aligns with the PR's objective of reducing build time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing performance benchmarks or test results rg -A 5 "cells.*800.*800|cells.*100.*100" --type pyLength of output: 2381
Script:
#!/bin/bash # Search for documentation about grid sizes and similar test files echo "=== Documentation and comments about grid sizes ===" rg -i "grid.*size|cell.*size|resolution" --type md echo -e "\n=== Similar test files in harris directory ===" fd -p "tests/functional/harris/.*\.py$" --exec head -n 50 {}Length of output: 2812
| if cpp.mpi_rank() == 0: | ||
| print(LOAD_BALANCE, "diag timestamps:", timestamps) |
There was a problem hiding this comment.
Fix undefined variables.
The following variables are used but not defined:
timestampsis referenced but not definedcppis used but commented out in the imports
+timestamps = np.linspace(0, final_time, num=10) # Adjust num as needed
+cpp = cpp_lib()
+startMPI()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
41-41: Undefined name cpp
(F821)
42-42: Undefined name timestamps
(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d_100_x_100.py (1)
Line range hint
1-134: Architecture feedback on test optimization.The changes align well with the PR objective of optimizing nightly builds by:
- Reducing computational requirements through smaller grid size
- Simplifying the simulation setup by removing multi-level refinement
- Improving code organization by moving common configuration to a base module
However, we should carefully consider:
- The trade-off between performance and accuracy with the reduced resolution
- The impact of disabled refinement on capturing important physical features
- The potential effects on existing workflows due to changed SCOPE_TIMING default
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/functional/harris/harris_2d.py(4 hunks)tests/functional/harris/harris_2d_100_x_100.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/harris_2d.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
tests/functional/harris/harris_2d_100_x_100.py (5)
Line range hint
82-95: LGTM: Comprehensive diagnostics coverage.The plot function provides thorough diagnostics including:
- Magnetic field divergence
- MPI rank distribution
- Particle density
- Magnetic field components
- Current density
37-40: Verify simulation accuracy with reduced grid size.The grid size has been significantly reduced from 800x800 to 100x100. While this will improve performance for nightly builds, we should ensure:
- The simulation remains numerically stable
- The results are still meaningful for validation purposes
Let's check if similar grid sizes are used in other tests:
✅ Verification successful
Grid size reduction is appropriate for nightly builds
The reduced 100x100 grid size aligns well with existing test configurations (e.g., 200x400 in harris_2d.py) and should maintain numerical stability while improving build performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for grid size configurations in other tests rg -A 2 "cells\s*=\s*\(" tests/functional/Length of output: 433
57-58: Review impact of disabled refinement.The refinement has been disabled and max_nbr_levels reduced to 1. This might affect the simulation's ability to resolve fine-scale features in regions where higher resolution is needed.
Let's analyze the usage of refinement in other Harris tests:
✅ Verification successful
Refinement configuration appears intentional
The disabled refinement in this test is consistent with its coarser grid setup (dl=0.40) compared to harris_2d.py (dl=0.2) which maintains refinement. The max_nbr_levels=1 setting is consistent across both tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check refinement settings in other Harris tests rg -B 2 -A 2 "refinement\s*=" tests/functional/harris/Length of output: 747
16-16: Consider impact of changing SCOPE_TIMING default.The default value for SCOPE_TIMING has been changed from "True" to "False". While this might improve performance, it could affect existing workflows that depend on timing data.
Let's check if other tests rely on this default:
Also applies to: 20-30
70-70: Verify configuration migration to base module.The configuration has been moved to the base module. Ensure that all necessary parameters are properly transferred and that the base configuration is compatible with all test variations.
Let's check the base configuration:
✅ Verification successful
Base configuration migration successfully verified
The test case properly inherits the complex physics setup from the base module while maintaining the ability to customize simulation parameters for its specific 100x100 configuration. The base configuration demonstrates compatibility with different grid sizes and simulation parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Examine the base configuration implementation cat tests/functional/harris/harris_2d.pyLength of output: 3587
Script:
#!/bin/bash # Examine the specific test configuration cat tests/functional/harris/harris_2d_100_x_100.pyLength of output: 3768
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/functional/harris/harris_2d_100_x_100.py(2 hunks)
⏰ 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 (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
tests/functional/harris/harris_2d_100_x_100.py (4)
16-16: LGTM! Good modular design.Moving common Harris simulation logic to a separate module improves code reusability and maintainability.
70-70: Verify base configuration implementation.The configuration has been moved to
base.config(). Let's ensure all necessary parameters are properly set in the base implementation.✅ Verification successful
Base configuration implementation is complete and correct
The base implementation in
harris_2d.pyincludes all necessary physical and numerical parameters, with proper validation and diagnostic setup.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check base configuration implementation echo "Checking base configuration..." cat tests/functional/harris/harris_2d.py 2>/dev/null || echo "Base configuration file not found"Length of output: 3658
37-40: Consider using 200x200 cells for nightly builds.The current reduction to 100x100 cells is more aggressive than the suggested 200x200 configuration mentioned in the PR comments. While this reduces build time, it might not provide sufficient resolution for meaningful results.
Consider the trade-off between:
- Build time reduction (current 100x100)
- Meaningful test coverage (suggested 200x200)
- Original configuration (800x800)
20-30: Verify SCOPE_TIMING requirements in the codebase.Let's verify that the documented requirements for scope timing match the actual implementation.
✅ Verification successful
Documentation accurately reflects scope timing requirements
The documented requirements for scope timing have been verified against the codebase implementation:
- PHARE_SCOPE_TIMING environment variable check is implemented in phare.hpp
- withPhlop CMake option is properly integrated in core/CMakeLists.txt
- PHARE_LOG_LEVEL defaults to 0 (off) in logger.hpp as documented
- phlop integration is handled through the subprojects system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify scope timing requirements echo "Checking PHARE_LOG_LEVEL definition..." rg -l "PHARE_LOG_LEVEL" src/core/logger.hpp echo "Checking scope timing implementation..." rg -l "PHARE_SCOPE_TIMING" src/phare/phare.hpp echo "Checking phlop dependency..." cat subprojects/phlop/meson.build 2>/dev/null || echo "phlop not found in subprojects"Length of output: 480
Script:
#!/bin/bash # Check implementation details echo "=== Logger Implementation ===" rg "PHARE_LOG_LEVEL" src/core/logger.hpp -A 5 echo "=== Scope Timing Implementation ===" rg "PHARE_SCOPE_TIMING" src/phare/phare.hpp -A 5 echo "=== CMake Configuration ===" fd CMakeLists.txt --exec grep -l "withPhlop" echo "=== Python Dependencies ===" fd "requirements.txt|setup.py|pyproject.toml" --exec cat {}Length of output: 3331
Script:
#!/bin/bash echo "=== CMake withPhlop Implementation ===" cat src/core/CMakeLists.txt echo "=== Searching for phlop in all cmake files ===" fd -e cmake -e txt -H CMake --exec grep -l "phlop"Length of output: 3206
| # refinement="tagging", | ||
| # max_nbr_levels=1, |
There was a problem hiding this comment.
💡 Codebase verification
Commented parameters are actively used in other Harris tests
The commented-out parameters are actively used in other test files:
harris_2d.pyuses both parameters:refinement="tagging", max_nbr_levels=1tdtagged/td1dtagged.pyuses different values:max_nbr_levels=3
No evidence found of these parameters being moved to a base configuration. Removing them might affect test consistency with other Harris simulations.
🔗 Analysis chain
Verify impact of disabled refinement parameters.
The commented-out parameters might affect simulation accuracy:
# refinement="tagging"
# max_nbr_levels=1Please clarify if disabling these parameters is intentional and how it affects the simulation results.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of refinement parameters in other tests
echo "Checking refinement parameter usage..."
rg -l "refinement.*tagging" tests/functional/
rg -l "max_nbr_levels" tests/functional/
Length of output: 455
Script:
#!/bin/bash
# Look for base config file
echo "Searching for base config..."
fd "base.config"
# Check actual parameter usage
echo -e "\nChecking parameter usage in test files..."
rg "refinement.*=.*tagging" tests/functional/ -B2 -A2
rg "max_nbr_levels.*=.*\d+" tests/functional/ -B2 -A2
Length of output: 2761
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d_100_x_100.py (1)
20-30: Improve documentation structure for scope timing requirements.While the documentation is comprehensive, it could be more structured. Consider using a bullet-point format for better readability:
""" For scope timings to work: - The env var PHARE_SCOPE_TIMING must be == "1" (or "true") - See src/phare/phare.hpp - CMake must be configured with: -DwithPhlop=ON - And a LOG_LEVEL must be defined via compile args: -DPHARE_LOG_LEVEL=1 - Or change the default value in src/core/logger.hpp - And phlop must be available on PYTHONPATH either from subprojects - or install phlop via pip + 1. Environment Variables: + - PHARE_SCOPE_TIMING must be "1" or "true" (see src/phare/phare.hpp) + + 2. Build Requirements: + - CMake flag: -DwithPhlop=ON + - Compile arg: -DPHARE_LOG_LEVEL=1 (or modify default in src/core/logger.hpp) + + 3. Runtime Requirements: + - phlop must be available on PYTHONPATH (via subprojects or pip install) """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/functional/harris/harris_2d_100_x_100.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
tests/functional/harris/harris_2d_100_x_100.py (2)
56-57: Clarify handling of refinement parameters.The commented parameters and their relocation require attention:
# refinement="tagging" # max_nbr_levels=1These parameters are actively used in other Harris tests. If they've been moved to
base.config(), please document this in the code. If not, their removal might affect test consistency.#!/bin/bash # Check if parameters are defined in base config echo "Checking base.config implementation..." rg -A 10 "def config" tests/functional/harris/harris_2d.pyAlso applies to: 69-69
37-38: Verify simulation accuracy with reduced parameters.The simulation has been significantly scaled down:
- Grid size reduced from 800x800 to 100x100 cells
- Final time reduced from 50 to 40
While this aligns with the PR objective of optimizing nightly builds, please verify that:
- The reduced scale maintains sufficient accuracy for the Harris sheet simulation
- Key physical phenomena are still captured at this resolution
Also applies to: 45-45
✅ Verification successful
Reduced simulation parameters appear to be acceptable
The 100x100 grid size is reasonable compared to the reference implementation (200x400) in
harris_2d.py, and no minimum size requirements were found in the codebase. The basic physics validation checks are maintained, suggesting the simulation should still capture the essential phenomena.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation criteria or similar configurations echo "Checking for validation criteria..." rg -l "validation|accuracy|resolution.*study" tests/functional/harris/ # Look for other Harris configurations echo -e "\nChecking other Harris configurations..." rg "cells.*=.*\([0-9]+,\s*[0-9]+\)" tests/functional/harris/Length of output: 440
Script:
#!/bin/bash # Check for test assertions and validation echo "Checking test assertions..." rg -A 5 "assert|test.*harris|check" tests/functional/harris/ # Look for documentation and comments echo -e "\nChecking documentation..." rg -A 3 "^#|\"\"\"" tests/functional/harris/ # Check configuration validation echo -e "\nChecking configuration validation..." rg "validate|check.*config|min.*size|grid.*size" tests/Length of output: 3948
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/functional/harris/harris_2d.py (3)
22-26: Consider injecting time parameters instead of using globals.The function relies on global variables
time_stepandfinal_time. This makes testing and reuse more difficult.Consider this refactor:
-def default_timestamps(): - dt = 10 * time_step - nt = final_time / dt + 1 - timestamps = dt * np.arange(nt) +def default_timestamps(time_step, final_time): + dt = 10 * time_step + nt = final_time / dt + 1 + return dt * np.arange(nt)🧰 Tools
🪛 Ruff (0.8.2)
25-25: Local variable
timestampsis assigned to but never usedRemove assignment to unused variable
timestamps(F841)
51-56: Document the purpose of the seed parameter.The new seed parameter affects simulation reproducibility. Consider adding docstring explaining its impact.
def config(sim = None, timestamps = None, seed = 12334): + """Configure Harris sheet simulation. + + Args: + sim: Optional simulation instance. If None, creates new instance. + timestamps: Optional timestamps array. If None, uses default timestamps. + seed: Random seed for particle initialization (default: 12334). + """
Line range hint
58-136: Add documentation for Harris sheet model equations.The mathematical functions implement complex plasma physics calculations but lack documentation explaining the equations and their physical significance.
Consider adding docstrings explaining:
- The physical meaning of each function
- The units of input/output values
- References to relevant physics papers
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/functional/harris/harris_2d.py(3 hunks)tests/functional/harris/harris_2d_100_x_100.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/functional/harris/harris_2d.py
25-25: Local variable timestamps is assigned to but never used
Remove assignment to unused variable timestamps
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
tests/functional/harris/harris_2d.py (1)
Line range hint
28-49: Verify the commented-out boundary_types parameter.The
boundary_types="periodic"parameter is commented out. Please verify if this is intentional as it might affect simulation behavior.Consider extracting simulation parameters to a configuration object.
The function contains many hardcoded simulation parameters which makes it less flexible for different test scenarios.
Consider creating a configuration class or dictionary to manage these parameters.
✅ Verification successful
The commented boundary_types parameter is safe to ignore.
The parameter defaults to "periodic" when not specified, which is the same value that was commented out. This is consistent with similar 2D test configurations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check usage of boundary_types in other Harris tests rg "boundary_types.*periodic" tests/functional/harris/Length of output: 130
Script:
#!/bin/bash # Search for any boundary_types usage in the codebase rg "boundary_types" -A 2 # Check for documentation or configuration files fd README -e md -X catLength of output: 13506
🧰 Tools
🪛 Ruff (0.8.2)
25-25: Local variable
timestampsis assigned to but never usedRemove assignment to unused variable
timestamps(F841)
tests/functional/harris/harris_2d_100_x_100.py (3)
37-37: Verify performance vs accuracy trade-off.Significant changes to simulation parameters:
- Grid size reduced from (800,800) to (100,100)
- Refinement parameters commented out
These changes will improve performance but might affect simulation accuracy. Please verify that the results remain valid for your use case.
Also applies to: 53-54
66-66: LGTM! Good refactoring of configuration logic.Moving the complex configuration to
base.configimproves code reuse and maintainability.
20-30: Verify impact of disabling scope timing by default.The default value for PHARE_SCOPE_TIMING has been changed to "False". While the documentation is comprehensive, this change might affect existing workflows or CI pipelines that rely on timing data.
✅ Verification successful
Default scope timing change is safe to proceed
The change to disable scope timing by default is consistent with the rest of the codebase, where it defaults to "false" in both Python and C++ implementations. No CI pipelines or test assertions depend on this feature, and the comprehensive documentation clearly explains how to enable timing when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for timing dependencies in CI rg "PHARE_SCOPE_TIMING" .github/workflows/Length of output: 42
Script:
#!/bin/bash # Check for timing-related code and configurations rg "PHARE_SCOPE_TIMING" -g '!tests/functional/harris/harris_2d_100_x_100.py' # Check for any timing assertions in tests rg "assert.*tim(e|ing)" tests/Length of output: 3342
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/functional/harris/CMakeLists.txt(1 hunks)tests/functional/harris/harris_2d.py(3 hunks)tests/functional/harris/harris_2d_100_x_100.py(2 hunks)tests/functional/harris/harris_2d_100_x_100_slow.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/CMakeLists.txt
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/functional/harris/harris_2d.py
25-25: Local variable timestamps is assigned to but never used
Remove assignment to unused variable timestamps
(F841)
⏰ 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 (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
tests/functional/harris/harris_2d_100_x_100.py (4)
16-30: LGTM! Well-documented scope timing requirements.The documentation clearly outlines the prerequisites for enabling scope timing functionality.
53-54: Commented parameters are actively used in other Harris tests.The commented-out parameters are actively used in other test files:
harris_2d.pyuses both parameters:refinement="tagging", max_nbr_levels=1tdtagged/td1dtagged.pyuses different values:max_nbr_levels=3No evidence found of these parameters being moved to a base configuration. Removing them might affect test consistency with other Harris simulations.
37-39: Verify simulation accuracy with reduced grid size.The grid size has been significantly reduced from (800, 800) to (100, 100). While this aligns with the PR objective to optimize nightly builds, please ensure that this reduction doesn't compromise simulation accuracy for the Harris sheet problem.
✅ Verification successful
Grid size reduction appears acceptable based on existing tests.
The codebase contains multiple Harris sheet tests using (100, 100) cells, suggesting this is an established and validated configuration. Additionally, there's precedent for various grid sizes, including a (200, 400) configuration, indicating flexibility in the spatial resolution requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation tests or documentation discussing grid size requirements rg -l "grid.*size|cells.*resolution" tests/functional/harris/ rg -l "convergence.*study|accuracy.*test" tests/functional/harris/Length of output: 132
Script:
#!/bin/bash # Search for other Harris sheet test configurations fd harris_2d tests/functional/ --exec head -n 50 {} # Search for comments or documentation about simulation parameters rg -i "harris.*param|simulation.*param|grid.*param" tests/functional/harris/Length of output: 3844
66-66: Verify parameter propagation to base configuration.The configuration has been refactored to use
base.config(sim, timestamps). Please ensure that all necessary parameters are correctly propagated through this call.✅ Verification successful
Parameter propagation to base configuration is correct
The
base.config(sim, timestamps)call correctly passes the required parameters. The base implementation accepts these parameters with optional defaults, ensuring backward compatibility and proper configuration handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for required parameters in base configuration ast-grep --pattern 'def config($_, $_) { $$$ return $_ }'Length of output: 63
Script:
#!/bin/bash # Search for base.config implementation with Python syntax ast-grep --pattern 'def config(sim, timestamps): $$$' # Also search for the file content to see the implementation rg -l "def config" --type pyLength of output: 768
Script:
#!/bin/bash # Look for base.config implementation rg -p "def config" tests/functional/harris/base.py # Check the context in harris_2d_100_x_100.py rg -p "base.config" tests/functional/harris/harris_2d_100_x_100.py -B 5 -A 5Length of output: 539
Script:
#!/bin/bash # Find imports in the harris test file rg -p "^import|^from.*import" tests/functional/harris/harris_2d_100_x_100.py # Search for base.py files in the test directory fd "base.py" tests/functional/Length of output: 489
Script:
#!/bin/bash # Get the config implementation from harris_2d.py rg -p "def config" tests/functional/harris/harris_2d.py -B 5 -A 15Length of output: 634
tests/functional/harris/harris_2d.py (2)
Line range hint
28-49: LGTM! Well-structured default setup.The
default_setup()function provides a clear and configurable base simulation setup.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Local variable
timestampsis assigned to but never usedRemove assignment to unused variable
timestamps(F841)
Line range hint
51-145: LGTM! Enhanced configuration flexibility.The updated
configfunction with optional parameters and reproducible seed value improves code reusability and test reliability.tests/functional/harris/harris_2d_100_x_100_slow.py (1)
1-141: LGTM! Well-structured slow test variant.The new test file provides a complementary test case with different parameters, following the same structure and best practices as the existing tests.
| def default_timestamps(): | ||
| dt = 10 * time_step | ||
| nt = final_time / dt + 1 | ||
| timestamps = dt * np.arange(nt) | ||
|
|
There was a problem hiding this comment.
Fix unused variable warning.
The timestamps variable is assigned but never used. Consider either returning or removing it.
def default_timestamps():
dt = 10 * time_step
nt = final_time / dt + 1
- timestamps = dt * np.arange(nt)
+ return dt * np.arange(nt)📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def default_timestamps(): | |
| dt = 10 * time_step | |
| nt = final_time / dt + 1 | |
| timestamps = dt * np.arange(nt) | |
| def default_timestamps(): | |
| dt = 10 * time_step | |
| nt = final_time / dt + 1 | |
| return dt * np.arange(nt) |
🧰 Tools
🪛 Ruff (0.8.2)
25-25: Local variable timestamps is assigned to but never used
Remove assignment to unused variable timestamps
(F841)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d_100_x_100_slow.py (1)
36-38: Consider parameterizing time step configuration.This file duplicates much of the code from
harris_2d_100_x_100.pybut uses a different time step (0.001 vs 0.005). Consider:
- Moving common configuration to a shared module
- Parameterizing the time step value
-time_step = 0.001 +TIME_STEP = float(os.getenv("HARRIS_TIME_STEP", "0.001")) +time_step = TIME_STEP
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/functional/harris/harris_2d_100_x_100.py(3 hunks)tests/functional/harris/harris_2d_100_x_100_slow.py(1 hunks)
⏰ 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: Analyze (python)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (6)
tests/functional/harris/harris_2d_100_x_100.py (5)
19-29: Well-documented scope timing configuration!The comprehensive documentation clearly explains the requirements for enabling scope timing functionality.
36-38: Grid size reduction aligns with performance goals.The reduction from 800x800 to 100x100 cells should significantly improve build times, which aligns with the PR objective of optimizing nightly builds.
52-53: Verify impact of disabled refinement parameters.The commented-out parameters might affect simulation accuracy:
# refinement="tagging" # max_nbr_levels=1
135-140: Good error handling for plotting module!The try-except block provides clear feedback with installation instructions when the plotting module is not available.
65-65: Verify base configuration implementation.The simulation configuration has been modularized by moving implementation details to
base.config. Please ensure that all necessary parameters are properly passed through and configured in the base module.✅ Verification successful
Base configuration implementation verified successfully
The base configuration is properly implemented with:
- Default simulation setup with appropriate grid parameters
- Configurable timestamps based on time step parameters
- Support for parameter overrides through optional arguments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check base.config implementation echo "Checking base.config implementation..." rg -A 10 "def config.*sim.*timestamps" tests/functional/harris/harris_2d.pyLength of output: 490
Script:
#!/bin/bash # Check default setup and timestamps implementations echo "Checking default setup and timestamps..." rg -B 5 -A 10 "def default_setup" tests/functional/harris/harris_2d.py rg -B 5 -A 10 "def default_timestamps" tests/functional/harris/harris_2d.pyLength of output: 902
tests/functional/harris/harris_2d_100_x_100_slow.py (1)
116-127: Well-structured test implementation!The test class follows best practices:
- Proper inheritance from SimulatorTest
- Thorough cleanup in tearDown
- Clear separation of concerns
| time_step=time_step, | ||
| final_time=final_time, | ||
| cells=cells, | ||
| dl=(0.40, 0.40), | ||
| # refinement="tagging", | ||
| # max_nbr_levels=1, | ||
| nesting_buffer=1, | ||
| clustering="tile", | ||
| tag_buffer="1", | ||
| hyper_resistivity=0.002, | ||
| resistivity=0.001, | ||
| diag_options={ | ||
| "format": "phareh5", | ||
| "options": {"dir": diag_dir, "mode": "overwrite"}, | ||
| }, | ||
| ) | ||
|
|
||
| sim = base.config(sim, timestamps) | ||
|
|
||
| for quantity in ["density", "bulkVelocity"]: | ||
| ph.FluidDiagnostics(quantity=quantity, write_timestamps=timestamps) | ||
|
|
||
| ph.FluidDiagnostics( | ||
| quantity="density", write_timestamps=timestamps, population_name="protons" | ||
| ) | ||
|
|
||
| if LOAD_BALANCE: | ||
| ph.LoadBalancer(active=True, auto=True, mode="nppc", tol=0.05) | ||
|
|
||
| return sim | ||
|
|
||
|
|
||
| def plot_file_for_qty(qty, time): | ||
| return f"{plot_dir}/harris_{qty}_t{time}.png" | ||
|
|
||
|
|
||
| def plot(diag_dir): | ||
| run = Run(diag_dir) | ||
| for time in timestamps: | ||
| run.GetDivB(time).plot( | ||
| filename=plot_file_for_qty("divb", time), | ||
| plot_patches=True, | ||
| vmin=1e-11, | ||
| vmax=2e-10, | ||
| ) | ||
| run.GetRanks(time).plot( | ||
| filename=plot_file_for_qty("Ranks", time), | ||
| plot_patches=True, | ||
| ) | ||
| run.GetN(time, pop_name="protons").plot( | ||
| filename=plot_file_for_qty("N", time), | ||
| plot_patches=True, | ||
| ) | ||
| for c in ["x", "y", "z"]: | ||
| run.GetB(time).plot( | ||
| filename=plot_file_for_qty(f"b{c}", time), | ||
| qty=f"{c}", | ||
| plot_patches=True, | ||
| ) | ||
| run.GetJ(time).plot( | ||
| filename=plot_file_for_qty("jz", time), | ||
| qty="z", | ||
| plot_patches=True, | ||
| vmin=-2, | ||
| vmax=2, | ||
| ) | ||
|
|
||
|
|
||
| class HarrisTest(SimulatorTest): | ||
| def __init__(self, *args, **kwargs): | ||
| super(HarrisTest, self).__init__(*args, **kwargs) | ||
| self.simulator = None | ||
|
|
||
| def tearDown(self): | ||
| super(HarrisTest, self).tearDown() | ||
| if self.simulator is not None: | ||
| self.simulator.reset() | ||
| self.simulator = None | ||
| ph.global_vars.sim = None | ||
|
|
||
| def test_run(self): | ||
| self.register_diag_dir_for_cleanup(diag_dir) | ||
| Simulator(config()).run().reset() | ||
| if cpp.mpi_rank() == 0: | ||
| plot(diag_dir) | ||
|
|
||
| if SCOPE_TIMING: | ||
| try: | ||
| from tools.python3 import plotting as m_plotting | ||
|
|
||
| m_plotting.plot_run_timer_data(diag_dir, cpp.mpi_rank()) | ||
| except ImportError: | ||
| print("Phlop not found - install with: `pip install phlop`") | ||
|
|
||
| cpp.mpi_barrier() | ||
| return self | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| HarrisTest().test_run().tearDown() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate test files.
This file is nearly identical to harris_2d_100_x_100.py. Consider:
- Creating a base test class with common functionality
- Deriving specific test cases with different parameters
- Using parameterized testing
This would reduce code duplication and make maintenance easier.
Example refactor:
# harris_2d_base_test.py
class HarrisBaseTest(SimulatorTest):
def __init__(self, time_step, diag_suffix=""):
super().__init__()
self.time_step = time_step
self.diag_suffix = diag_suffix
# ... common setup ...
# harris_2d_100_x_100.py
class HarrisFastTest(HarrisBaseTest):
def __init__(self):
super().__init__(time_step=0.005)
# harris_2d_100_x_100_slow.py
class HarrisSlowTest(HarrisBaseTest):
def __init__(self):
super().__init__(time_step=0.001, diag_suffix="_slow")
Closes #896
Summary by CodeRabbit
Bug Fixes
Tests
Chores