Change default oxidation states from SMACT14 to ICSD24 #346
Conversation
WalkthroughThe changes involve the introduction of a new parameter, Changes
Possibly related PRs
Suggested labels
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #346 +/- ##
==========================================
+ Coverage 77.44% 77.56% +0.12%
==========================================
Files 31 31
Lines 2589 2599 +10
==========================================
+ Hits 2005 2016 +11
+ Misses 584 583 -1 ☔ View full report in Codecov by Sentry. |
![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 (2)
smact/utils/crystal_space/generate_composition_with_smact.py (1)
59-59: Enhance parameter documentationThe docstring should specify all valid options for
oxidation_states_setand explain the implications of each choice.Consider expanding the documentation:
- oxidation_states_set (str): the oxidation states set to use. Options are "smact14", "icsd16", "icsd24", "pymatgen_sp" or a filepath to a custom oxidation states list. For reproducing the Faraday Discussions results, use "smact14". + oxidation_states_set (str): The oxidation states set to use for filtering compositions. Options: + - "smact14": Original SMACT oxidation states, suitable for reproducing Faraday Discussions results + - "icsd16": ICSD 2016 oxidation states + - "icsd24": ICSD 2024 oxidation states (default, recommended for new analyses) + - "pymatgen_sp": Pymatgen's oxidation states + - filepath: Path to a custom oxidation states list filesmact/tests/test_core.py (1)
Line range hint
402-407: Consider adding assertions for all oxidation state sets.While the test covers several oxidation state sets for MgB2, it would be more comprehensive to test all available sets consistently.
- self.assertFalse(smact.screening.smact_validity("MgB2", oxidation_states_set="smact14")) - self.assertTrue(smact.screening.smact_validity("MgB2", oxidation_states_set="icsd16")) - self.assertFalse(smact.screening.smact_validity("MgB2", oxidation_states_set="pymatgen_sp")) - self.assertTrue(smact.screening.smact_validity("MgB2", oxidation_states_set="wiki")) - self.assertFalse(smact.screening.smact_validity("MgB2", oxidation_states_set=TEST_OX_STATES)) + oxidation_sets_results = { + "smact14": False, + "icsd16": True, + "icsd24": True, + "pymatgen_sp": False, + "wiki": True, + TEST_OX_STATES: False + } + for ox_set, expected in oxidation_sets_results.items(): + with self.subTest(ox_set=ox_set): + result = smact.screening.smact_validity("MgB2", oxidation_states_set=ox_set) + self.assertEqual(result, expected, f"Failed for oxidation set {ox_set}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
docs/tutorials/crystal_space.ipynb(1 hunks)docs/tutorials/smact_validity_of_GNoMe.ipynb(3 hunks)smact/screening.py(3 hunks)smact/tests/test_core.py(3 hunks)smact/tests/test_utils.py(1 hunks)smact/utils/crystal_space/generate_composition_with_smact.py(3 hunks)
🔇 Additional comments (9)
smact/utils/crystal_space/generate_composition_with_smact.py (2)
47-47: LGTM: Parameter addition follows Python best practices
The new parameter oxidation_states_set is added with a sensible default value of "icsd24".
109-112: LGTM: Consistent parameter forwarding
The oxidation_states_set parameter is correctly forwarded to the smact_filter function.
docs/tutorials/crystal_space.ipynb (1)
105-105: LGTM: Ensures reproducibility with published results
The explicit use of oxidation_states_set="smact14" maintains consistency with the Faraday Discussions paper results.
smact/tests/test_utils.py (2)
99-103: LGTM: Well-structured test data
The test data is clearly organised with expected results for each oxidation state set.
104-120: LGTM: Comprehensive test coverage
The test implementation:
- Uses subtests for clear failure isolation
- Verifies both oxidation state sets
- Checks DataFrame type, size, and content
- Validates file saving
- Includes proper cleanup
smact/tests/test_core.py (2)
338-347: Well-structured test data organization!
The dictionary structure provides a clear separation of test cases for different oxidation state sets, making it easier to maintain and extend.
351-357: Good use of subTest for parameterized testing!
Using subTest is the correct approach for testing multiple oxidation state sets, as it provides better test isolation and clearer failure reporting.
smact/screening.py (1)
338-339: Verify the impact of changing default oxidation states.
The change of default oxidation state set from "smact14" to "icsd24" is a breaking change that could affect existing code.
Also applies to: 441-442
docs/tutorials/smact_validity_of_GNoMe.ipynb (1)
347-347: Good practice: Explicit oxidation state set specification.
Explicitly setting oxidation_states_set="smact14" ensures reproducibility and maintains backward compatibility with previous results.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
smact/tests/test_core.py (2)
339-359: LGTM: Comprehensive test coverage for both oxidation state setsThe test effectively validates both SMACT14 and ICSD24 oxidation states using parameterised subtests.
Consider extracting the test data to a separate fixture or constant to improve maintainability:
OXIDATION_STATES_TEST_DATA = { "smact14": { "thresh_2": [ (("Na", "Fe", "Cl"), (1, -1, -1), (2, 1, 1)), (("Na", "Fe", "Cl"), (1, 1, -1), (1, 1, 2)), ] }, "icsd24": {"thresh_2": [(("Na", "Fe", "Cl"), (1, 1, -1), (1, 1, 2))]} }
403-404: LGTM: Good test case selectionThe test effectively demonstrates how MgB2 validity varies with different oxidation state sets.
Consider adding a docstring to explain why MgB2 was chosen as a test case and what makes it particularly suitable for testing different oxidation state sets.
smact/screening.py (1)
487-493: LGTM: Consistent implementation of oxidation state selectionThe implementation correctly handles both explicit and default oxidation state selection.
Consider updating the error message to explicitly mention "icsd24" as the default:
- f'{oxidation_states_set} is not valid. Enter either "smact14", "icsd16", "icsd24", "pymatgen_sp","wiki" or a filepath to a textfile of oxidation states.' + f'{oxidation_states_set} is not valid. Enter either "icsd24" (default), "smact14", "icsd16", "pymatgen_sp", "wiki" or a filepath to a textfile of oxidation states.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
smact/__init__.py(2 hunks)smact/screening.py(5 hunks)smact/tests/test_core.py(4 hunks)
🔇 Additional comments (3)
smact/tests/test_core.py (1)
50-51: LGTM: Test maintains backwards compatibility
The test correctly validates both SMACT14 and default oxidation states for the Rb element.
smact/__init__.py (2)
61-61: LGTM: Clear version-specific documentation
The documentation clearly explains that oxidation states differ between versions 3.0 and earlier.
182-182: LGTM: Consistent implementation of new defaults
The change correctly implements ICSD24 as the default oxidation states source.
Let's check for any direct usage of the oxidation_states attribute:
✅ Verification successful
Change to ICSD24 oxidation states is safe and well-integrated
The codebase analysis shows that the change to use ICSD24 as the default oxidation states source is properly integrated:
- The Element class maintains backward compatibility by preserving access to other oxidation state sets (SMACT14, ICSD16, SP, Wiki)
- The screening module correctly handles all oxidation state sources including ICSD24
- All direct usages of oxidation states are properly abstracted through the Element class interface
- Test coverage exists for oxidation state functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of oxidation_states attribute
ast-grep --pattern 'Element($$$).oxidation_states'
Length of output: 52
Script:
#!/bin/bash
# Search for any usage of oxidation_states in the codebase
rg -l "oxidation_states"
# Search for Element class usage patterns
rg "Element\(" -A 2
# Search for specific pattern where oxidation states might be accessed
rg "\.oxidation_states"
Length of output: 10171
| @@ -335,8 +335,7 @@ def smact_filter( | |||
| threshold: int | None = 8, | |||
| stoichs: list[list[int]] | None = None, | |||
| species_unique: bool = True, | |||
| oxidation_states_set: str = "smact14", | |||
| comp_tuple: bool = False, | |||
| oxidation_states_set: str = "icsd24", | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking change is properly documented but requires version update
The change from "smact14" to "icsd24" as the default oxidation state set is well-documented in the code with:
- Clear deprecation notice mentioning this is a breaking change for SMACT 3.0
- Detailed documentation of all available oxidation state sets
- Explicit instructions for backwards compatibility
- Tests that cover both oxidation state sets
However, this breaking change requires:
- Increment of the major version number to 3.0 as per the documentation
- Migration of the change notice to a more visible location like README.md
🔗 Analysis chain
Breaking change: Default oxidation states updated
The change of default oxidation state set from "smact14" to "icsd24" is a breaking change that could affect existing code.
Let's check if this change is properly documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the change is documented in README or changelog
rg -i "breaking.*change.*oxidation" README.md CHANGELOG.md
rg -i "default.*oxidation.*state" README.md CHANGELOG.md
Length of output: 225
Script:
#!/bin/bash
# Let's check for any documentation files in the repository
fd -e md -e rst
# Check for any mentions of oxidation states in documentation
rg -i "oxidation.*state" -g "*.{md,rst}"
# Check the specific function to understand the impact
ast-grep --pattern 'def smact_filter($$$) { $$$ }'
Length of output: 5444
Script:
#!/bin/bash
# Let's check the specific file for documentation about the oxidation state sets
rg -A 5 "icsd24|smact14" smact/screening.py
# Check the oxidation states module for details about these sets
rg -A 5 "icsd24|smact14" smact/oxidation_states.py
# Look for any test files that might be affected
fd -e py -e test test | xargs rg "smact14"
Length of output: 5961
Change default oxidation states from SMACT14 to ICSD24
Description
This PR introduces a breaking change to SMACT, where the decade long oxidation states default are now changed to a data-mined set of oxidation states from the ICSD. Main considerations for these changes were to have a list of oxidation states which reflect what has been observed in experimental structures.
smact_filterandsmact_validitynow usesicsd24as the default oxidation statessmact14andicsd24oxidation statesgenerate_compositions_with_smactnow allows users to specify the oxidation state listssmact14andicsd24oxidation states are now tested for the above functionsmact14oxidation states for reproducibility of the results reported in the Faraday Discussions publicationType of change
How Has This Been Tested?
New tests were added and run locally to verify that the breaking changes don't break the codebase. Where tests failed, this arose due to
oxidation_states_setnot having an argument being supplied. This has been fixed by setting the argument equal tosmact14and additionally testing foricsd24Test Configuration:
Reviewers
N/A
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
smact_filterandgenerate_composition_with_smactfunctions to include new oxidation states.