Define needs_u_correction(comp: CompositionLike) -> set[str] utility function
#3703
Conversation
janosh
added
enhancement
WalkthroughThe update brings a mix of enhancements across different components: the pre-commit configuration, pymatgen library updates, and test suite improvements. It introduces a new argument for the Changes
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: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .pre-commit-config.yaml (1 hunks)
- pymatgen/entries/compatibility.py (7 hunks)
- tests/entries/test_compatibility.py (4 hunks)
Additional comments: 8
.pre-commit-config.yaml (1)
- 11-11: The update to
ruff-pre-commitfromv0.3.3tov0.3.4is noted and seems appropriate for staying up-to-date with dependencies. However, the addition of--unsafe-fixesto theruffhook arguments warrants careful consideration. While this argument can enable more aggressive automatic fixes, it may also introduce changes that could potentially alter the intended behavior of the code. It's recommended to thoroughly review the changes made by this option during the pre-commit phase to ensure they align with the project's standards and expectations.pymatgen/entries/compatibility.py (5)
- 51-54: The assertion here checks for consistency in Hubbard U corrections for O and F. Consider adding a more informative error message to guide users on how to resolve the issue if this assertion fails. For example, you could suggest checking the
MP2020Compatibility.yamlfile for correctness.- 1472-1495: The
needs_u_correctionfunction could benefit from an enhanced docstring that provides more context about its return value. Specifically, it would be helpful to explain the significance of the returned set of elements and how it indicates the need for U-correction. This additional information will make the function more understandable to users who may not be familiar with the concept of U-corrections.- 802-808: The addition of a deprecation warning in the
MaterialsProjectCompatibilityclass is a good practice for informing users about outdated functionality. Ensure that all references to this class in documentation and example code are updated to guide users towards the recommended alternatives.- 909-918: In the
MaterialsProject2020Compatibilityclass, consider adding error handling or logging for cases where the configuration file might be missing or corrupted. This will improve the robustness of the class and provide clearer feedback to users encountering issues with the configuration.- 1470-1472: The
MaterialsProjectAqueousCompatibilityclass introduces a significant enhancement with the aqueous energy referencing scheme. Ensure thorough testing, especially for theget_adjustmentsmethod, to validate the accuracy of the adjustments applied and to handle edge cases gracefully. Consider adding unit tests that cover a variety of compositions, including edge cases like compositions without explicit oxidation states.tests/entries/test_compatibility.py (2)
- 2044-2063: The test class
TestCorrectionErrors2020Compatibilityand its methodtest_errorsare correctly implemented to check the correction uncertainties of various entries. Good use ofapproxfor floating-point comparison.- 2044-2065: The standalone function
test_needs_u_correctionis correctly implemented to test various compositions against expected results for needing U correction. Proper use ofpytest.mark.parametrizefor testing multiple scenarios.
| expected_u = float(u_settings.get(symbol, 0)) | ||
| actual_u = float(calc_u.get(symbol, 0)) | ||
| if actual_u != expected_u: | ||
| raise CompatibilityError(f"Invalid U value of {actual_u:.1f} on {symbol}, expected {expected_u:.1f}") | ||
| raise CompatibilityError(f"Invalid U value of {actual_u:.3} on {symbol}, expected {expected_u:.3}") |
There was a problem hiding this comment.
In the MaterialsProject2020Compatibility.get_adjustments method, the comparison between expected_u and actual_u uses floating-point equality. Due to the nature of floating-point arithmetic, this might lead to unexpected results. Consider using a tolerance-based comparison method to avoid potential issues with floating-point precision.
- if actual_u != expected_u:
+ if not math.isclose(actual_u, expected_u, rel_tol=1e-9):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.
| expected_u = float(u_settings.get(symbol, 0)) | |
| actual_u = float(calc_u.get(symbol, 0)) | |
| if actual_u != expected_u: | |
| raise CompatibilityError(f"Invalid U value of {actual_u:.1f} on {symbol}, expected {expected_u:.1f}") | |
| raise CompatibilityError(f"Invalid U value of {actual_u:.3} on {symbol}, expected {expected_u:.3}") | |
| expected_u = float(u_settings.get(symbol, 0)) | |
| actual_u = float(calc_u.get(symbol, 0)) | |
| if not math.isclose(actual_u, expected_u, rel_tol=1e-9): | |
| raise CompatibilityError(f"Invalid U value of {actual_u:.3} on {symbol}, expected {expected_u:.3}") |
|
Sorry to be late to the party here @janosh . This looks useful to me. It's easy to forget that we only correct certain TMs when they occur as oxides or fluorides so it'll be nice to have a check like this. Thanks for contributing! |
Checks if a
CompositionLikeis Hubbard U-corrected in the Materials Project 2020 GGA/GGA+U mixing scheme.would be great to get other's opinions on this @rkingsbury @JaGeo @Andrew-S-Rosen