Define ElementType enum in core/periodic_table.py
#3726
Conversation
WalkthroughThe recent updates focus on optimizing and extending functionality across several scripts and modules within a development environment. Key enhancements include improved data structure handling, such as the use of tuples and 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 (
|
janosh
added
enhancement
![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
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (6)
- dev_scripts/chemenv/get_plane_permutations_optimized.py (1 hunks)
- dev_scripts/update_pt_data.py (1 hunks)
- pymatgen/core/composition.py (2 hunks)
- pymatgen/core/periodic_table.py (1 hunks)
- tests/core/test_composition.py (1 hunks)
- tests/core/test_periodic_table.py (2 hunks)
Additional Context Used
Additional comments not posted (8)
dev_scripts/update_pt_data.py (1)
165-165: The use ofsetdefaulthere is a good improvement, making the code more concise and potentially more efficient by avoiding explicit checks. This is a nice example of Pythonic code.dev_scripts/chemenv/get_plane_permutations_optimized.py (1)
282-282: Changing the condition check from a list to a tuple is a good practice for static value checks, offering a slight performance improvement due to the immutability of tuples.tests/core/test_periodic_table.py (1)
607-612: The test for theElementTypeenum checks its existence and the number of categories, which is a good approach for verifying the enum's integration. However, the enum definition itself is not included in the provided code.tests/core/test_composition.py (3)
676-678: The test case forComposition("Y3N@C80")correctly asserts that the composition created from the string is equivalent to the composition created from the dictionary. This is a good practice for ensuring that the parser correctly handles special formulas like metallofullerenes.
681-696: The tests forcontains_element_typemethod are comprehensive, covering various element types and blocks. However, it's important to ensure that theElementTypeenum and its integration are correctly implemented in theCompositionclass for these tests to be valid. Additionally, the use ofpytest.raisesto check for an invalid category is a good practice for error handling tests.
715-731: Thetest_remove_chargesmethod correctly tests the functionality of removing charges from compositions. It's good to see that both simple and mixed-valence compositions are tested. This ensures that the method works as expected in various scenarios. However, it would be beneficial to add a comment explaining the purpose of callingremove_chargesmultiple times (lines 720 and 723) to clarify if it's testing idempotency.Consider adding a comment explaining the repeated calls to
remove_chargesto clarify the test's intent, such as testing the method's idempotency.pymatgen/core/composition.py (2)
21-21: The import statement has been modified to includeElementTypefrompymatgen.core.periodic_table. This change is consistent with the PR's objective to introduce and utilize theElementTypeenum for categorizing elements. Ensure that theElementTypeenum is properly defined and implemented in the referenced module to avoid import errors.
504-507: The modification in thecontains_element_typemethod to use a list comprehension for extracting values fromElementTypeforallowed_categoriesis a good approach for ensuring that the method can dynamically check against the newly introduced categories of elements. However, it's important to ensure that theElementTypeenum members have a.valueattribute that correctly represents the category strings expected by this method. Additionally, the error message has been updated to guide users towards valid categories, which enhances usability.One potential improvement is to cache the
allowed_categorieslist if this method is called frequently, to avoid recomputing the list every time the method is invoked.Consider caching the
allowed_categorieslist if performance becomes a concern due to frequent calls tocontains_element_type.
| @unique | ||
| class ElementType(Enum): | ||
| """Enum for element types.""" | ||
|
|
||
| noble_gas = "noble_gas" # He, Ne, Ar, Kr, Xe, Rn | ||
| transition_metal = "transition_metal" # Sc-Zn, Y-Cd, La-Hg, Ac-Cn | ||
| post_transition_metal = "post_transition_metal" # Al, Ga, In, Tl, Sn, Pb, Bi, Po | ||
| rare_earth_metal = "rare_earth_metal" # Ce-Lu, Th-Lr | ||
| metal = "metal" | ||
| metalloid = "metalloid" # B, Si, Ge, As, Sb, Te, Po | ||
| alkali = "alkali" # Li, Na, K, Rb, Cs, Fr | ||
| alkaline = "alkaline" # Be, Mg, Ca, Sr, Ba, Ra | ||
| halogen = "halogen" # F, Cl, Br, I, At | ||
| chalcogen = "chalcogen" # O, S, Se, Te, Po | ||
| lanthanoid = "lanthanoid" # La-Lu | ||
| actinoid = "actinoid" # Ac-Lr | ||
| quadrupolar = "quadrupolar" | ||
| s_block = "s-block" | ||
| p_block = "p-block" | ||
| d_block = "d-block" | ||
| f_block = "f-block" |
There was a problem hiding this comment.
The ElementType enum introduces clear categorizations for elements based on their properties and positions in the periodic table. However, the inclusion of a generic 'metal' category could be ambiguous, as many other categories also represent metals. Consider refining this category or providing additional documentation on its intended use to avoid confusion.
Additionally, while the enum member names are descriptive, Python's convention for enum names is to use uppercase letters with underscores separating words (e.g., NOBLE_GAS instead of noble_gas). Adhering to this convention would improve consistency with Python standards.
- noble_gas = "noble_gas"
+ NOBLE_GAS = "noble_gas"
- transition_metal = "transition_metal"
+ TRANSITION_METAL = "transition_metal"
- post_transition_metal = "post_transition_metal"
+ POST_TRANSITION_METAL = "post_transition_metal"
- rare_earth_metal = "rare_earth_metal"
+ RARE_EARTH_METAL = "rare_earth_metal"
- metal = "metal"
+ METAL = "metal"
- metalloid = "metalloid"
+ METALLOID = "metalloid"
- alkali = "alkali"
+ ALKALI = "alkali"
- alkaline = "alkaline"
+ ALKALINE = "alkaline"
- halogen = "halogen"
+ HALOGEN = "halogen"
- chalcogen = "chalcogen"
+ CHALCOGEN = "chalcogen"
- lanthanoid = "lanthanoid"
+ LANTHANOID = "lanthanoid"
- actinoid = "actinoid"
+ ACTINOID = "actinoid"
- quadrupolar = "quadrupolar"
+ QUADRUPOLAR = "quadrupolar"
- s_block = "s-block"
+ S_BLOCK = "s-block"
- p_block = "p-block"
+ P_BLOCK = "p-block"
- d_block = "d-block"
+ D_BLOCK = "d-block"
- f_block = "f-block"
+ F_BLOCK = "f-block"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.
| @unique | |
| class ElementType(Enum): | |
| """Enum for element types.""" | |
| noble_gas = "noble_gas" # He, Ne, Ar, Kr, Xe, Rn | |
| transition_metal = "transition_metal" # Sc-Zn, Y-Cd, La-Hg, Ac-Cn | |
| post_transition_metal = "post_transition_metal" # Al, Ga, In, Tl, Sn, Pb, Bi, Po | |
| rare_earth_metal = "rare_earth_metal" # Ce-Lu, Th-Lr | |
| metal = "metal" | |
| metalloid = "metalloid" # B, Si, Ge, As, Sb, Te, Po | |
| alkali = "alkali" # Li, Na, K, Rb, Cs, Fr | |
| alkaline = "alkaline" # Be, Mg, Ca, Sr, Ba, Ra | |
| halogen = "halogen" # F, Cl, Br, I, At | |
| chalcogen = "chalcogen" # O, S, Se, Te, Po | |
| lanthanoid = "lanthanoid" # La-Lu | |
| actinoid = "actinoid" # Ac-Lr | |
| quadrupolar = "quadrupolar" | |
| s_block = "s-block" | |
| p_block = "p-block" | |
| d_block = "d-block" | |
| f_block = "f-block" | |
| @unique | |
| class ElementType(Enum): | |
| """Enum for element types.""" | |
| NOBLE_GAS = "noble_gas" # He, Ne, Ar, Kr, Xe, Rn | |
| TRANSITION_METAL = "transition_metal" # Sc-Zn, Y-Cd, La-Hg, Ac-Cn | |
| POST_TRANSITION_METAL = "post_transition_metal" # Al, Ga, In, Tl, Sn, Pb, Bi, Po | |
| RARE_EARTH_METAL = "rare_earth_metal" # Ce-Lu, Th-Lr | |
| METAL = "metal" | |
| METALLOID = "metalloid" # B, Si, Ge, As, Sb, Te, Po | |
| ALKALI = "alkali" # Li, Na, K, Rb, Cs, Fr | |
| ALKALINE = "alkaline" # Be, Mg, Ca, Sr, Ba, Ra | |
| HALOGEN = "halogen" # F, Cl, Br, I, At | |
| CHALCOGEN = "chalcogen" # O, S, Se, Te, Po | |
| LANTHANOID = "lanthanoid" # La-Lu | |
| ACTINOID = "actinoid" # Ac-Lr | |
| QUADRUPOLAR = "quadrupolar" | |
| S_BLOCK = "s-block" | |
| P_BLOCK = "p-block" | |
| D_BLOCK = "d-block" | |
| F_BLOCK = "f-block" |
defined as above to avoid breaking changes in
Composition.contains_element_type. definitely think this could be improved. we can make (breaking) changes later up to 1 or 2 releases down the road when adoption is still lowpymatgen/pymatgen/core/composition.py
Lines 492 to 512 in 3a49dd9
Summary by CodeRabbit
ElementTypeenum to classify elements such as noble gas, transition metal, metalloid, etc.parse_shannon_radiifunction for better dictionary initialization.contains_element_typemethod for improved element type validation.