fix(pt ut): make separated uts deterministic #4162
Conversation
WalkthroughWalkthroughThe pull request introduces a new parameter, Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (19)
source/tests/pt/model/water/se_atten.json (1)
27-28: LGTM! Consider parameterizing the seed value.The addition of the "seed" property to the descriptor configuration is a good step towards ensuring reproducibility and aligns well with the PR objective of making separated unit tests deterministic. The consistency of the seed value (1) between the descriptor and fitting_net sections is commendable.
To improve flexibility, consider parameterizing the seed value. This would allow easier changes for different test scenarios without modifying the JSON file. For example:
"seed": "${SEED:-1}"This suggestion uses a placeholder that can be replaced at runtime, defaulting to 1 if not specified. Implement this change only if it aligns with your current configuration management practices.
source/tests/pt/model/test_se_t.py (3)
66-66: LGTM! Consider adding a default value for the seed parameter.The addition of the
seedparameter to theDescrptSeTconstructor is a good step towards making the unit tests deterministic. This change aligns well with the PR objective.Consider adding a default value for the
seedparameter in the function signature. This would maintain backwards compatibility and allow for easier testing with different seeds if needed. For example:def test_consistency(self, seed=GLOBAL_SEED):
135-135: LGTM! Consider adding a default value for the seed parameter.The addition of the
seedparameter to theDescrptSeTconstructor in thetest_jitfunction is consistent with the changes made totest_consistencyand aligns with the PR objective of making unit tests deterministic.As suggested for
test_consistency, consider adding a default value for theseedparameter in the function signature:def test_jit(self, seed=GLOBAL_SEED):This change would maintain consistency between the two test functions and provide flexibility for future testing scenarios.
Line range hint
1-135: Overall, the changes look good and achieve the PR objective.The modifications to both
test_consistencyandtest_jitfunctions are consistent and contribute to making the unit tests more deterministic. The use ofGLOBAL_SEEDensures reproducibility across test runs.To further improve the code:
- Consider extracting the
GLOBAL_SEEDinto a configuration file or environment variable. This would allow easier management of the seed value across the entire test suite.- If not already done, ensure that the
GLOBAL_SEEDis documented in the project's testing guidelines or README, explaining its purpose and how it contributes to deterministic test results.source/tests/pt/model/test_se_atten_v2.py (3)
70-70: LGTM: Addition of GLOBAL_SEED parameterThe addition of
GLOBAL_SEEDas a parameter to theDescrptSeAttenV2initialization enhances the reproducibility of the test, which is in line with the PR objective.Consider adding a comment explaining the purpose of using
GLOBAL_SEEDhere, for better code readability. For example:seed=GLOBAL_SEED, # Ensure deterministic behavior across test runs
142-142: LGTM: Addition of GLOBAL_SEED parameterThe addition of
GLOBAL_SEEDas a parameter to theDescrptSeAttenV2initialization in thetest_jitmethod is consistent with the changes made intest_consistencyand aligns with the PR objective of making tests deterministic.Similar to the suggestion for
test_consistency, consider adding a comment explaining the purpose of usingGLOBAL_SEEDhere:seed=GLOBAL_SEED, # Ensure deterministic behavior across test runs
Line range hint
1-142: Summary: Enhancing test determinism with GLOBAL_SEEDThe changes in this file consistently apply
GLOBAL_SEEDto theDescrptSeAttenV2initialization in bothtest_consistencyandtest_jitmethods. This implementation aligns well with the PR objective of making separated unit tests deterministic.These changes will improve the reproducibility of test results, making it easier to debug issues and ensure consistent behavior across different test runs.
To further improve the implementation:
- Consider adding a brief comment in the file docstring explaining the use of
GLOBAL_SEEDfor test determinism.- If
GLOBAL_SEEDis used in other test files, ensure consistent implementation across the test suite.source/tests/pt/model/test_ener_fitting.py (4)
68-68: LGTM! Consider extracting GLOBAL_SEED to a constant.The addition of the
seedparameter to theInvarFittingconstructor is a good step towards making the tests deterministic. This change aligns well with the PR objective.To improve code readability and maintainability, consider extracting
GLOBAL_SEEDto a constant at the top of the file:SEED = GLOBAL_SEED # Then use it in the test: seed=SEED,This makes it easier to update the seed value for all tests in this file if needed in the future.
172-172: LGTM! Ensure consistency with SEED constant.The addition of the
seedparameter here is consistent with the changes in thetest_consistencyfunction and aligns with the PR objective.If you implement the suggestion to extract
GLOBAL_SEEDto aSEEDconstant, ensure you use it here as well for consistency:seed=SEED,
182-185: LGTM! Good use of consistent seeding.The addition of the
seedparameter to theInvarFittingconstructor and the use of the same seed for the random number generator is an excellent approach to ensure test determinism.If you implement the suggestion to extract
GLOBAL_SEEDto aSEEDconstant, ensure you use it here as well for consistency:SEED = GLOBAL_SEED # Then in the test: seed=SEED, ) rng = np.random.default_rng(SEED)This change maintains the good practice of using the same seed for both the
InvarFittingconstructor and the random number generator while improving code readability.
Line range hint
1-191: Summary: Excellent progress on making tests deterministicThe changes in this file consistently add a
seedparameter to theInvarFittingconstructor calls in various test functions. This aligns perfectly with the PR objective of making separated unit tests deterministic.Key points:
- The use of
GLOBAL_SEEDensures consistency across different test runs.- The changes don't alter the logic of the tests but enhance reproducibility.
- In the
test_get_setfunction, the same seed is used for both theInvarFittingconstructor and the random number generator, which is a good practice.To further improve the code:
- Consider extracting
GLOBAL_SEEDto aSEEDconstant at the top of the file for better readability and maintainability.- Ensure that all randomness in the tests is seeded, including any random operations in the tested functions themselves.
- Document the purpose of using a fixed seed in the test file's docstring to explain the deterministic nature of these tests to other developers.
source/tests/pt/model/test_descriptor_se_r.py (2)
66-66: LGTM! Consider documenting the seed usage.The addition of the
seedparameter to theDescrptSeRconstructor is a good step towards making the tests deterministic. This change aligns well with the PR objective.Consider adding a brief comment explaining the purpose of setting the seed, e.g.:
# Set seed for reproducibility seed=GLOBAL_SEED,This would help other developers understand the importance of this change at a glance.
134-134: LGTM! Consider consistent formatting.The addition of the
seedparameter to theDescrptSeRconstructor is correct and aligns with the PR objective.For consistency with the surrounding code, consider aligning the
seedparameter with the other parameters:seed=GLOBAL_SEED,This minor adjustment would improve code readability and maintain a consistent style throughout the file.
source/tests/pt/model/test_embedding_net.py (1)
159-159: LGTM: Use of GLOBAL_SEED improves test determinism.Replacing the hardcoded seed value with
GLOBAL_SEEDenhances the test's consistency and aligns with the PR objective of making unit tests deterministic. This change will ensure that the random number generation in this test is consistent with other tests using the same global seed.Consider adding a brief comment explaining the purpose of using
GLOBAL_SEEDhere, for improved code readability. For example:seed=GLOBAL_SEED, # Use global seed for consistent random number generation across testssource/tests/pt/model/test_dpa1.py (2)
74-74: LGTM! Consider documenting the purpose of the seed parameter.The addition of the
seedparameter withGLOBAL_SEEDis a good step towards making the unit tests deterministic. This change aligns well with the PR objective.Consider adding a brief comment explaining the purpose of the
seedparameter, e.g.:seed=GLOBAL_SEED, # Ensures deterministic behavior in testsThis would help other developers understand the importance of this parameter at a glance.
215-215: LGTM! Consider extracting common test parameters.The addition of the
seedparameter withGLOBAL_SEEDto the JIT test is consistent with the previous changes and maintains deterministic behavior across different test methods.To improve code maintainability and reduce duplication, consider extracting common test parameters (including the seed) into a separate method or fixture. This could look like:
def get_test_params(self): return { "rcut": self.rcut, "rcut_smth": self.rcut_smth, "sel": self.sel, "nt": self.nt, "seed": GLOBAL_SEED, # ... other common parameters ... } # Then in your test methods: dd0 = DescrptDPA1(**self.get_test_params(), precision=prec, resnet_dt=idt, ...)This approach would make it easier to ensure consistency across different test methods and simplify future updates to common parameters.
source/tests/pt/model/test_permutation.py (3)
91-91: LGTM! Consider using a constant for the seed value.The addition of the "seed" parameter to the descriptor configuration is a good practice for ensuring reproducibility.
Consider defining a constant for the seed value (e.g.,
DEFAULT_SEED = 1) at the module level and using it across all configurations. This would make it easier to change the seed value globally if needed in the future.
271-271: LGTM! Comprehensive seed implementation in hybrid model.The addition of the "seed" parameter to both descriptor configurations in the model_hybrid is thorough and consistent with other changes, ensuring reproducibility across all components of the hybrid model.
Consider using a single seed value for the entire hybrid model configuration, rather than specifying it separately for each descriptor. This could be achieved by moving the seed parameter to the top level of the hybrid model configuration. This approach would ensure that all components of the hybrid model use the same seed, potentially simplifying configuration management.
Also applies to: 304-304
91-91: Overall LGTM! Consider global seed management.The consistent addition of the "seed" parameter across all model configurations is a significant improvement for ensuring reproducibility in the tests. This change will help in debugging and maintaining consistent behavior across different runs and environments.
To further improve the code:
- Define a global constant for the seed value at the module level, e.g.,
DEFAULT_SEED = 1.- Use this constant throughout all model configurations.
- Consider adding a function to generate model configurations with a customizable seed, allowing for easy manipulation of the seed value across all models when needed.
Example:
DEFAULT_SEED = 1 def add_seed_to_config(config, seed=DEFAULT_SEED): if isinstance(config, dict): for key, value in config.items(): if isinstance(value, dict): config[key] = add_seed_to_config(value, seed) if "type" in config and "seed" not in config: config["seed"] = seed return config # Usage model_se_e2_a = add_seed_to_config({ "type_map": ["O", "H", "B"], "descriptor": { "type": "se_e2_a", # ... other parameters ... }, # ... other configurations ... })This approach would centralize seed management, making it easier to modify or randomize seeds for all models simultaneously when needed.
Also applies to: 159-159, 212-212, 241-241, 271-271, 304-304
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- source/tests/pt/model/models/dpa1.json (1 hunks)
- source/tests/pt/model/models/dpa2.json (1 hunks)
- source/tests/pt/model/test_descriptor_se_r.py (3 hunks)
- source/tests/pt/model/test_dipole_fitting.py (6 hunks)
- source/tests/pt/model/test_dpa1.py (3 hunks)
- source/tests/pt/model/test_dpa2.py (4 hunks)
- source/tests/pt/model/test_embedding_net.py (2 hunks)
- source/tests/pt/model/test_ener_fitting.py (3 hunks)
- source/tests/pt/model/test_permutation.py (6 hunks)
- source/tests/pt/model/test_polarizability_fitting.py (6 hunks)
- source/tests/pt/model/test_property_fitting.py (8 hunks)
- source/tests/pt/model/test_se_atten_v2.py (3 hunks)
- source/tests/pt/model/test_se_e2_a.py (3 hunks)
- source/tests/pt/model/test_se_t.py (2 hunks)
- source/tests/pt/model/water/se_atten.json (1 hunks)
Additional comments not posted (40)
source/tests/pt/model/models/dpa1.json (1)
24-25: LGTM! Consider making the seed configurable.The addition of the "seed" property is a good practice for ensuring reproducibility in model training and testing. This change aligns well with the PR objective of making unit tests deterministic.
Consider making the seed value configurable rather than hardcoded. This would allow for easy adjustment in different environments (e.g., development, testing, production) without modifying the configuration file. You could use environment variables or command-line arguments to set the seed value.
Let's verify if the "seed" property in the "fitting_net" section was intentionally left unchanged:
If this returns
true, it suggests that both seeds are intentionally set to the same value. Iffalse, you might want to consider aligning them or explaining why they differ.Verification successful
Verified that both "descriptor.seed" and "fitting_net.seed" are correctly set to the same value.
The synchronization of the seed values ensures consistent behavior across different components of the model.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if "seed" property exists in both "descriptor" and "fitting_net" sections # and if they have the same value jq '.descriptor.seed == .fitting_net.seed' source/tests/pt/model/models/dpa1.jsonLength of output: 88
source/tests/pt/model/test_se_atten_v2.py (1)
19-21: LGTM: Import of GLOBAL_SEEDThe import of
GLOBAL_SEEDfrom theseedmodule is correctly placed and aligns with the PR objective of making separated unit tests deterministic.source/tests/pt/model/test_descriptor_se_r.py (2)
66-66: Overall assessment: Changes improve test determinism.The addition of the
seed=GLOBAL_SEEDparameter to theDescrptSeRconstructor calls in all three test methods (test_consistency,test_load_stat, andtest_jit) is a positive change that aligns well with the PR objective of making unit tests deterministic.These modifications should enhance the reproducibility of test results across different runs, which is crucial for maintaining a stable test suite. The consistent implementation across all methods demonstrates a thorough approach to addressing the issue.
To further improve the code:
- Consider adding brief comments explaining the purpose of setting the seed.
- Ensure consistent formatting of the new parameter across all methods.
- Verify that the JIT compilation works correctly with the added seed parameter.
These changes contribute to better code quality and test reliability.
Also applies to: 134-134, 185-185
185-185: LGTM! Verify JIT compilation with seed.The addition of the
seedparameter to theDescrptSeRconstructor in thetest_jitmethod is correct and consistent with the changes in other test methods.To ensure that the JIT compilation works correctly with the added seed parameter, please run the following verification script:
This verification step will help ensure that the addition of the seed parameter doesn't introduce any issues with JIT compilation.
source/tests/pt/model/test_se_e2_a.py (4)
63-63: Excellent addition of the seed parameter for deterministic testing!The inclusion of
seed=GLOBAL_SEEDin the DescrptSeA constructor is a crucial step towards ensuring reproducible test results. This change aligns perfectly with the PR objective of making unit tests deterministic.
117-117: Great consistency in applying the seed parameter!Adding
seed=GLOBAL_SEEDto the DescrptSeA constructor withold_impl=Trueensures that both the new and old implementations use the same seed. This is crucial for fair and consistent comparisons between the two implementations, further enhancing the determinism and reliability of the tests.
173-173: Excellent consistency in applying the seed parameter to JIT testing!The addition of
seed=GLOBAL_SEEDto the DescrptSeA constructor in thetest_jitmethod ensures that the JIT-compiled version of the model also uses a deterministic seed. This change is crucial for maintaining consistency across different testing scenarios and contributes significantly to the overall goal of making all aspects of testing deterministic.
Line range hint
1-173: Summary: Excellent implementation of deterministic seeding across all test scenarios!The changes made to this file consistently apply the
GLOBAL_SEEDto all instances of DescrptSeA creation, including the new implementation, old implementation, and JIT-compiled version. This comprehensive approach ensures that all test scenarios are deterministic and reproducible, which is crucial for maintaining reliable and consistent test results.These changes align perfectly with the PR objective of making unit tests deterministic and represent a significant improvement in the testing framework's robustness.
source/tests/pt/model/test_embedding_net.py (2)
42-44: LGTM: Import of GLOBAL_SEED enhances test determinism.The addition of
GLOBAL_SEEDimport aligns with the PR objective of making separated unit tests deterministic. This change will help ensure consistent random number generation across tests, improving reproducibility.
Line range hint
1-259: Summary: Changes effectively contribute to test determinism.The modifications in this file, namely the import and use of
GLOBAL_SEED, successfully contribute to the PR objective of making separated unit tests deterministic. These changes are minimal yet impactful, enhancing the reproducibility of thetest_consistencymethod without altering its core logic. The implementation is clean and well-integrated with the existing code.source/tests/pt/model/test_dpa1.py (2)
Line range hint
1-215: Overall, the changes effectively address the PR objective.The consistent addition of the
seedparameter usingGLOBAL_SEEDacross different test methods (consistency, old implementation, and JIT) is a solid approach to ensuring deterministic behavior in the unit tests. This change aligns well with the PR objective of making separated unit tests deterministic.Key points:
- Deterministic behavior is now ensured across different implementations and test methods.
- The use of a global seed (
GLOBAL_SEED) promotes consistency throughout the test suite.- The changes maintain backwards compatibility by including the seed in the old implementation test.
Suggestions for further improvement:
- Consider adding brief comments explaining the purpose of the seed parameter.
- Explore the possibility of extracting common test parameters to reduce code duplication and improve maintainability.
- Verify that the seed is being used consistently across all relevant parts of the test suite.
These changes represent a significant step towards more reliable and reproducible unit tests.
129-129: LGTM! Verify seed consistency across implementations.The addition of the
seedparameter withGLOBAL_SEEDto the old implementation is consistent with the previous change and supports the goal of deterministic testing.To ensure that the seed is being used consistently across both new and old implementations, consider adding an assertion to verify that the random states are identical after initialization. You can use the following script to check for any occurrences where the seed might not be used consistently:
If this script returns any results, it might indicate places where the seed is not being used consistently, which could lead to non-deterministic behavior.
Verification successful
Seed consistency across implementations confirmed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent use of GLOBAL_SEED across the file # Test: Search for occurrences of DescrptDPA1 without GLOBAL_SEED rg 'DescrptDPA1\([^)]*\)' source/tests/pt/model/test_dpa1.py | grep -v 'seed=GLOBAL_SEED'Length of output: 89
Script:
#!/bin/bash # Description: Verify that all DescrptDPA1 constructor calls include seed=GLOBAL_SEED # Using rg to find all DescrptDPA1 constructor calls rg 'DescrptDPA1\s*\([^)]*\)' source/tests/pt/model/test_dpa1.py | grep -v 'seed=GLOBAL_SEED' || echo "No instances without seed=GLOBAL_SEED found."Length of output: 191
source/tests/pt/model/test_dpa2.py (4)
23-25: LGTM: Import of GLOBAL_SEED for deterministic testingThe addition of the GLOBAL_SEED import is a good practice for ensuring reproducibility in tests. This change aligns well with the PR objective of making separated unit tests deterministic.
158-158: LGTM: Added GLOBAL_SEED to DescrptDPA2 constructor callsThe addition of the
seed=GLOBAL_SEEDparameter to the DescrptDPA2 constructor calls in both the new and old implementations is a positive change. This ensures that the DescrptDPA2 instances are initialized with a consistent random state, contributing to the deterministic behavior of the test. This modification aligns well with the PR objective of making the unit tests deterministic.Also applies to: 208-208
354-354: LGTM: Added GLOBAL_SEED to DescrptDPA2 constructor in test_jitThe addition of the
seed=GLOBAL_SEEDparameter to the DescrptDPA2 constructor call in the test_jit function is consistent with the changes made elsewhere in the file. This modification ensures deterministic behavior during JIT compilation testing, which is in line with the PR objective of making the unit tests deterministic.
Line range hint
1-354: Overall assessment: Changes effectively improve test determinismThe modifications made to this file consistently add the GLOBAL_SEED parameter to DescrptDPA2 constructor calls in various test functions. These changes effectively address the PR objective of making separated unit tests deterministic. The implementation is consistent throughout the file and should contribute to more reliable and reproducible test results.
source/tests/pt/model/test_dipole_fitting.py (7)
90-90: LGTM: Added seed parameter for deterministic testingThe addition of the
seedparameter withGLOBAL_SEEDvalue to theDipoleFittingNetinitialization is a good practice. This change aligns with the PR objective of making unit tests deterministic, ensuring consistent results across different test runs.
143-143: LGTM: Consistent use of seed parameterThe addition of the
seedparameter withGLOBAL_SEEDvalue to thisDipoleFittingNetinitialization is consistent with the previous change. This ensures deterministic behavior in JIT compilation tests as well.
185-185: LGTM: Consistent use of seed parameter in rotation testsThe addition of the
seedparameter withGLOBAL_SEEDvalue to theDipoleFittingNetinitialization in thetest_rotmethod is consistent with the previous changes. This ensures deterministic behavior in rotation invariance tests.
240-240: LGTM: Consistent use of seed parameter in permutation testsThe addition of the
seedparameter withGLOBAL_SEEDvalue to theDipoleFittingNetinitialization in thetest_permumethod is consistent with the previous changes. This ensures deterministic behavior in permutation invariance tests.
287-287: LGTM: Consistent use of seed parameter in translation testsThe addition of the
seedparameter withGLOBAL_SEEDvalue to theDipoleFittingNetinitialization in thetest_transmethod is consistent with the previous changes. This ensures deterministic behavior in translation invariance tests.
335-335: LGTM: Consistent use of seed parameter in TestDipoleModel setupThe addition of the
seedparameter withGLOBAL_SEEDvalue to theDipoleFittingNetinitialization in thesetUpmethod ofTestDipoleModelclass is consistent with the previous changes. This ensures deterministic behavior in the setup of theTestDipoleModelclass.
Line range hint
1-400: Overall assessment: Consistent implementation of deterministic testingThe changes made to this file are consistent and well-implemented. The
seedparameter has been added to allDipoleFittingNetinitializations, using theGLOBAL_SEEDconstant throughout. This approach ensures deterministic behavior across all tests, including consistency, JIT compilation, rotation, permutation, and translation invariance tests.These changes align perfectly with the PR objective of making unit tests deterministic, which will improve the reliability and reproducibility of the test suite. The core test logic remains unchanged, maintaining the integrity of the existing tests while enhancing their consistency.
Great job on implementing these changes systematically throughout the file!
source/tests/pt/model/test_polarizability_fitting.py (7)
80-80: LGTM: Added seed parameter for reproducibilityThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization is a good practice. This change aligns with the PR objective to make separated unit tests deterministic, which is crucial for ensuring consistent and reproducible test results across different runs.
147-147: LGTM: Consistent seed parameter additionThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization in thetest_jitmethod is consistent with the previous change. This demonstrates a systematic approach to making the tests deterministic across different methods, which is commendable.
191-191: LGTM: Seed parameter added for rotational invariance testThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization in thetest_rotmethod is consistent with previous changes. This is particularly important for rotational invariance testing, as it ensures that any random elements in the test (such as the rotation matrix generation) are reproducible, allowing for consistent verification of this critical property.
254-254: LGTM: Seed parameter added for permutation invariance testThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization in thetest_permumethod maintains consistency with previous changes. This is particularly valuable for permutation invariance testing, as it ensures that any random elements in the test are reproducible. This allows for consistent verification of the model's behavior under different atom orderings, which is a crucial property for physical systems.
305-305: LGTM: Seed parameter added for translational invariance testThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization in thetest_transmethod maintains consistency with previous changes. This is particularly important for translational invariance testing, as it ensures that any random elements in the test are reproducible. This allows for consistent verification of the model's behavior under different spatial translations, which is a fundamental property for physical systems.
355-355: LGTM: Seed parameter added to test class setupThe addition of the
seedparameter withGLOBAL_SEEDvalue to thePolarFittingNetinitialization in thesetUpmethod of theTestPolarModelclass is a good practice. This ensures that all tests within this class will use the same deterministic initialization, promoting consistency across the entire test suite. This change aligns well with the overall goal of making the tests more reproducible and reliable.
Line range hint
80-355: Summary: Excellent implementation of deterministic testingThe changes made in this file consistently add a
seedparameter withGLOBAL_SEEDvalue to variousPolarFittingNetinitializations across different test methods and classes. This implementation aligns perfectly with the PR objective of making separated unit tests deterministic. The changes cover all relevant test methods, including those for rotational, permutational, and translational invariance, as well as the general model setup.These modifications will significantly improve the reproducibility and reliability of the test suite, making it easier to debug issues and ensure consistent behavior across different environments. The use of a global seed is a good practice that ensures uniformity throughout the tests.
Overall, this is a well-executed change that should have a positive impact on the testing process and the overall quality of the codebase.
source/tests/pt/model/test_permutation.py (3)
159-159: LGTM! Consistent use of seed parameter.The addition of the "seed" parameter to the model_zbl descriptor configuration is consistent with other changes, ensuring reproducibility across different model types.
212-212: LGTM! Consistent seed parameter implementation.The addition of the "seed" parameter to the model_dpa2tebd descriptor configuration maintains consistency with other model configurations, ensuring reproducibility.
241-241: LGTM! Seed parameter added consistently.The addition of the "seed" parameter to the model_dpa1 descriptor configuration is in line with the changes made to other model configurations, ensuring consistent reproducibility across different model types.
source/tests/pt/model/test_property_fitting.py (7)
35-37: LGTM: New import for GLOBAL_SEED.The addition of the GLOBAL_SEED import is appropriate and follows the existing import structure. This constant will help ensure consistent seeding across tests, improving reproducibility.
84-84: LGTM: Added GLOBAL_SEED to PropertyFittingNet initialization.The addition of GLOBAL_SEED as a parameter to PropertyFittingNet initialization is a good practice. This ensures that the network is initialized consistently across test runs, contributing to the overall goal of making the tests deterministic.
153-153: LGTM: Added GLOBAL_SEED to PropertyFittingNet initialization in test_jit.The addition of GLOBAL_SEED as a parameter to PropertyFittingNet initialization in the test_jit method is consistent with the previous change. This ensures that the network is initialized consistently during JIT compilation tests, further contributing to test determinism.
207-207: LGTM: Added GLOBAL_SEED to PropertyFittingNet initialization in TestInvarianceRandomShift.The addition of GLOBAL_SEED as a parameter to PropertyFittingNet initialization in the TestInvarianceRandomShift class is consistent with previous changes. This ensures that the network is initialized consistently during rotation invariance tests, further enhancing test determinism.
330-330: LGTM: Added GLOBAL_SEED to PropertyFittingNet initialization in test_permu.The addition of GLOBAL_SEED as a parameter to PropertyFittingNet initialization in the test_permu method is consistent with previous changes. This ensures that the network is initialized consistently during permutation invariance tests, further contributing to overall test determinism.
429-429: LGTM: Added GLOBAL_SEED to PropertyFittingNet initialization in TestPropertyModel.The addition of GLOBAL_SEED as a parameter to PropertyFittingNet initialization in the TestPropertyModel class is consistent with previous changes. This ensures that the network is initialized consistently during PropertyModel tests, further enhancing overall test determinism.
Line range hint
1-458: Summary: Consistent implementation of GLOBAL_SEED across all tests.The changes in this file consistently add the GLOBAL_SEED parameter to all PropertyFittingNet initializations across various test classes and methods. This implementation successfully addresses the goal of making the tests deterministic by ensuring consistent seeding. The modifications are well-executed and should significantly improve the reproducibility of the test results.
Some potential benefits of these changes include:
- Improved debugging capabilities due to reproducible test results.
- Easier identification of regressions in model performance.
- More reliable comparison of test results across different environments or test runs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4162 +/- ##
=======================================
Coverage 83.43% 83.43%
=======================================
Files 537 537
Lines 52146 52146
Branches 3046 3046
=======================================
+ Hits 43507 43508 +1
Misses 7692 7692
+ Partials 947 946 -1 ☔ View full report in Codecov by Sentry. |
Fix failed uts in #4145 .
Summary by CodeRabbit
New Features
"seed"property to multiple JSON configuration files, enhancing control over randomness in model training and evaluation.Bug Fixes
Documentation