Correct units of Element.atomic_orbitals #3714
Conversation
esoteric-ephemera
requested review from
shyuep,
janosh and
mkhorton
as code owners
WalkthroughThe recent updates to the pymatgen library introduce a significant enhancement in representing atomic orbital energies. A new 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- pymatgen/core/periodic_table.py (3 hunks)
Additional comments: 4
pymatgen/core/periodic_table.py (4)
- 258-268: The addition of the
atomic_orbitals_eVproperty correctly addresses the issue of unit discrepancy in atomic orbital energies by providing these energies in electronvolts (eV). This change is well-documented and directly converts the energies from Hartree to eV using theHa_to_eVconstant, ensuring accuracy and consistency with the expectations set by the documentation.- 19-19: It's good practice to ensure that all imported modules and functions are used within the file. The imports are well-organized, and the use of
TYPE_CHECKINGto conditionally import for type hints is a good practice to avoid unnecessary runtime dependencies.- 77-77: The documentation for
atomic_orbitalsproperty clearly states that the energies are in Hartree units, which is crucial for avoiding confusion given the addition ofatomic_orbitals_eV. Maintaining clear and accurate documentation for each property ensures that users of the library can correctly interpret and use the provided data.- 258-268: The implementation of the
atomic_orbitals_eVproperty uses a dictionary comprehension to convert all orbital energies from Hartree to eV. This approach is both efficient and readable. However, it's important to ensure that theHa_to_eVconstant is accurately defined elsewhere in the codebase to guarantee correct conversion values.Verification successful
The
Ha_to_eVconstant is defined inpymatgen/core/units.pyas1 / const.physical_constants["electron volt-hartree relationship"][0], indicating that it relies on a physical constants dictionary for its value. This approach suggests a reliance on scientifically reputable sources for conversion factors, which is a good practice for ensuring accuracy. However, without direct access to theconst.physical_constantsdictionary, the exact numerical accuracy of the conversion cannot be independently verified here. Given the context, it's reasonable to trust the accuracy of this definition based on its source.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the accuracy of the Ha_to_eV constant definition. rg "Ha_to_eV = " --type pyLength of output: 132
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- pymatgen/core/periodic_table.py (3 hunks)
- tests/core/test_periodic_table.py (2 hunks)
- tests/core/test_units.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pymatgen/core/periodic_table.py
Additional comments: 6
tests/core/test_units.py (4)
- 12-12: The constant
Ha_to_eVis correctly defined and follows the naming convention of other constants in the module. However, ensure that the value assigned to this constant is accurate and consistent with authoritative sources.- 17-17: The constant
Ry_to_eVis added, which is essential for conversions between Rydberg and electron volts. Verify the accuracy of the conversion factor used here.- 22-23: The constants
amu_to_kgandbohr_to_angstromare added. It's crucial to verify these conversion factors for accuracy. Additionally, consider adding a brief comment above each constant to describe its purpose or source for future reference.- 30-35: The test
test_unit_conversionscorrectly asserts the values of the newly added constants. It's good practice to ensure these tests cover all edge cases and potential numerical precision issues. Consider adding a brief comment explaining the significance of each assertion for clarity.tests/core/test_periodic_table.py (2)
- 14-14: The import of
Ha_to_eVis correctly added to support the conversion of atomic orbital energies from Hartree to electron volts. This is a necessary step for the implementation ofatomic_orbitals_eV.- 246-286: The addition of
"atomic_orbitals_eV"to thekeystuple in thetest_attributesmethod is appropriate for testing the new property. This ensures that the property is correctly integrated and functional for all elements where applicable.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- pymatgen/core/periodic_table.py (3 hunks)
- tests/core/test_periodic_table.py (2 hunks)
- tests/core/test_units.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- pymatgen/core/periodic_table.py
- tests/core/test_periodic_table.py
- tests/core/test_units.py
The docstr for
pymatgen.core.Element.atomic_orbitalsindicates that the units of the LDA energies is eV. However the data inperiodic_table.jsonwas never converted to eV from Hartree, soelement.atomic_orbitalsprints energies in Hartree. (This is also true for pymatgen versions before the periodic table data was updated in #3542.)This was brought up in a matsci post.
This PR just adds an
Element.atomic_orbitals_eVproperty which converts the atomic orbitals energies to eV, and ensures no breaking changes.Summary by CodeRabbit
atomic_orbitals_eVproperty to include a calculation based onHa_to_eVconversion.Ha_to_eV,Ry_to_eV,bohr_to_angstrom,amu_to_kg) and tests for unit conversions.