Add oxidation states option to SMACT validity function #282
Conversation
WalkthroughSignificant changes include updating the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant smact_validity
participant OxidationStatesSet
User->>smact_validity: Call smact_validity(composition, oxidation_states_set)
smact_validity->>OxidationStatesSet: Select appropriate set based on parameter
OxidationStatesSet-->>smact_validity: Return oxidation states based on selection
smact_validity-->>User: Return validity result
Assessment against linked issues
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #282 +/- ##
===========================================
+ Coverage 74.50% 74.65% +0.14%
===========================================
Files 24 24
Lines 2083 2099 +16
===========================================
+ Hits 1552 1567 +15
- Misses 531 532 +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: 4
Outside diff range and nitpick comments (6)
smact/tests/test_doper.py (3)
1-1: Remove unused import:smact.The
smactmodule is imported but not used in this file. Removing unused imports helps keep the code clean and maintainable.- import smact
6-6: Remove unused import:smact.structure_prediction.mutation.The
mutationmodule fromsmact.structure_predictionis imported but not used in this file.- from smact.structure_prediction import mutation, utilities + from smact.structure_prediction import utilitiesTools
Ruff
6-6:
smact.structure_prediction.mutationimported but unusedRemove unused import:
smact.structure_prediction.mutation(F401)
14-14: Remove assignment to unused variablenum_dopants.The variable
num_dopantsis assigned but never used in thetest_dopant_predictionmethod.- num_dopants = 10Tools
Ruff
14-14: Local variable
num_dopantsis assigned to but never usedRemove assignment to unused variable
num_dopants(F841)
smact/dopant_prediction/doper.py (3)
3-3: Remove unused import:typing.Type.The
Typeclass from thetypingmodule is imported but not used in this file.- from typing import List, Optional, Tuple, Type + from typing import List, Optional, TupleTools
Ruff
3-3:
typing.Typeimported but unusedRemove unused import:
typing.Type(F401)
46-49: Improve error message for mutually exclusive parameters.The error message could be more informative by specifying the conflicting parameters.
- raise ValueError("Only one of filepath or embedding should be provided") + raise ValueError("The parameters 'filepath' and 'embedding' are mutually exclusive. Please provide only one of them.")
50-51: Add supported embeddings to the error message.The error message for unsupported embeddings should list the supported options.
- raise ValueError(f"Embedding {embedding} is not supported") + raise ValueError(f"Embedding {embedding} is not supported. Supported embeddings: 'skipspecies'")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- docs/conf.py (1 hunks)
- setup.py (1 hunks)
- smact/dopant_prediction/doper.py (11 hunks)
- smact/screening.py (2 hunks)
- smact/tests/test_doper.py (2 hunks)
Files skipped from review due to trivial changes (2)
- docs/conf.py
- setup.py
Additional context used
Ruff
smact/tests/test_doper.py
4-4:
smactimported but unusedRemove unused import:
smact(F401)
6-6:
smact.structure_prediction.mutationimported but unusedRemove unused import:
smact.structure_prediction.mutation(F401)
14-14: Local variable
num_dopantsis assigned to but never usedRemove assignment to unused variable
num_dopants(F841)
smact/dopant_prediction/doper.py
3-3:
typing.Typeimported but unusedRemove unused import:
typing.Type(F401)
GitHub Check: codecov/patch
smact/screening.py
[warning] 474-479: smact/screening.py#L474-L479
Added lines #L474 - L479 were not covered by tests
[warning] 482-483: smact/screening.py#L482-L483
Added lines #L482 - L483 were not covered by tests
[warning] 487-487: smact/screening.py#L487
Added line #L487 was not covered by tests
[warning] 490-490: smact/screening.py#L490
Added line #L490 was not covered by tests
Additional comments not posted (3)
smact/dopant_prediction/doper.py (2)
143-157: Ensure consistency in similarity/probability calculation.The
_calculate_species_sim_probmethod correctly differentiates between probability and similarity score. Ensure that the method is used consistently throughout the class.
Line range hint
158-288:
Validate logic inget_dopantsmethod.The
get_dopantsmethod includes new parameters and logic for calculating dopants. Ensure that the method is thoroughly tested, especially the new logic paths.smact/screening.py (1)
472-494: Ensure coverage for new oxidation states logic.The new logic for handling different oxidation states sets should be thoroughly tested to ensure correctness.
Tools
GitHub Check: codecov/patch
[warning] 474-479: smact/screening.py#L474-L479
Added lines #L474 - L479 were not covered by tests
[warning] 482-483: smact/screening.py#L482-L483
Added lines #L482 - L483 were not covered by tests
[warning] 487-487: smact/screening.py#L487
Added line #L487 was not covered by tests
[warning] 490-490: smact/screening.py#L490
Added line #L490 was not covered by tests
smact/screening.py
Outdated
| oxidation_states_set: Union[str, bytes, os.PathLike] = "default", | ||
| ) -> bool: | ||
| """Check if a composition is valid according to the SMACT rules. | ||
|
|
||
| Args: | ||
| composition (Union[pymatgen.core.Composition, str]): Composition/formula to check | ||
| use_pauling_test (bool): Whether to use the Pauling electronegativity test | ||
| include_alloys (bool): If True, compositions of metal elements will be considered valid | ||
| oxidation_states_set (Union[str, bytes, os.PathLike]): A string to choose which set of | ||
| oxidation states should be chosen for charge-balancing. Options are 'default', 'icsd', | ||
| 'pymatgen' and 'wiki' for the default, icsd, pymatgen structure predictor and Wikipedia | ||
| (https://en.wikipedia.org/wiki/Template:List_of_oxidation_states_of_the_elements) oxidation states respectively. | ||
| A filepath to an oxidation states text file can also be supplied. |
There was a problem hiding this comment.
Tip
Codebase Verification
Add tests for the new oxidation_states_set parameter.
The smact_validity function includes a new parameter oxidation_states_set. Ensure that this parameter is thoroughly tested, as it is not currently covered in the test suite.
- Add test cases in
smact/tests/test_core.pyto cover different values of theoxidation_states_setparameter.
Analysis chain
Add tests for the new oxidation_states_set parameter.
The smact_validity function includes a new parameter oxidation_states_set. Ensure that this parameter is thoroughly tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new parameter `oxidation_states_set` in the `smact_validity` function is tested.
# Test: Ensure that the new parameter is covered in the test suite.
rg --type python 'smact_validity'
Length of output: 64
Script:
#!/bin/bash
# Description: Verify the new parameter `oxidation_states_set` in the `smact_validity` function is tested.
# Test: Ensure that the new parameter is covered in the test suite.
rg 'smact_validity' --type py
Length of output: 600
Script:
#!/bin/bash
# Description: Verify the new parameter `oxidation_states_set` in the `smact_validity` function is tested.
# Test: Ensure that the new parameter is covered in the test suite.
rg 'smact_validity' smact/tests/test_core.py
Length of output: 425
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- smact/screening.py (2 hunks)
Additional comments not posted (1)
smact/screening.py (1)
441-445: Document the new parameter clearly.The new parameter
oxidation_states_setis well-documented, but ensure that the documentation is updated in all relevant places, including user guides and API references.
| @@ -430,13 +430,19 @@ def smact_validity( | |||
| composition: Union[pymatgen.core.Composition, str], | |||
| use_pauling_test: bool = True, | |||
| include_alloys: bool = True, | |||
| oxidation_states_set: Union[str, bytes, os.PathLike] = "default", | |||
There was a problem hiding this comment.
Add tests for new parameter.
The smact_validity function includes a new parameter oxidation_states_set. Ensure that this parameter is thoroughly tested, as it is not currently covered in the test suite.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- README.md (1 hunks)
- setup.py (1 hunks)
- smact/screening.py (2 hunks)
- smact/tests/test_core.py (1 hunks)
Files skipped from review due to trivial changes (1)
- setup.py
Additional context used
Markdownlint
README.md
104-104: null
Bare URL used(MD034, no-bare-urls)
GitHub Check: codecov/patch
smact/screening.py
[warning] 492-492: smact/screening.py#L492
Added line #L492 was not covered by tests
Additional comments not posted (2)
README.md (1)
101-101: Use HTTPS URL for installation.The change to use
https://instead ofgit://for the installation command is appropriate and enhances security.smact/tests/test_core.py (1)
414-431: Add tests for new parameter.The added test cases for the
smact_validityfunction with different oxidation state sets are necessary and well-implemented. Ensure that these tests cover all relevant scenarios for the new parameter.
| oxidation_states_set: Union[str, bytes, os.PathLike] = "default", | ||
| ) -> bool: | ||
| """Check if a composition is valid according to the SMACT rules. | ||
|
|
||
| Composition is considered valid if it passes the charge neutrality test and the Pauling electronegativity test. | ||
|
|
||
| Args: | ||
| composition (Union[pymatgen.core.Composition, str]): Composition/formula to check | ||
| composition (Union[pymatgen.core.Composition, str]): Composition/formula to check. This can be a pymatgen Composition object or a string. | ||
| use_pauling_test (bool): Whether to use the Pauling electronegativity test | ||
| include_alloys (bool): If True, compositions of metal elements will be considered valid | ||
| include_alloys (bool): If True, compositions which only contain metal elements will be considered valid without further checks. | ||
| oxidation_states_set (Union[str, bytes, os.PathLike]): A string to choose which set of | ||
| oxidation states should be chosen for charge-balancing. Options are 'default', 'icsd', | ||
| 'pymatgen' and 'wiki' for the default, icsd, pymatgen structure predictor and Wikipedia | ||
| (https://en.wikipedia.org/wiki/Template:List_of_oxidation_states_of_the_elements) oxidation states respectively. | ||
| A filepath to an oxidation states text file can also be supplied. |
There was a problem hiding this comment.
Add tests for the new oxidation_states_set parameter.
The smact_validity function includes a new parameter oxidation_states_set. Ensure that this parameter is thoroughly tested, as it is not currently covered in the test suite.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
| if oxidation_states_set == "default" or oxidation_states_set is None: | ||
| ox_combos = [e.oxidation_states for e in smact_elems] | ||
| elif oxidation_states_set == "icsd": | ||
| ox_combos = [e.oxidation_states_icsd for e in smact_elems] | ||
| elif oxidation_states_set == "pymatgen": | ||
| ox_combos = [e.oxidation_states_sp for e in smact_elems] | ||
| elif os.path.exists(oxidation_states_set): | ||
| ox_combos = [ | ||
| oxi_custom(e.symbol, oxidation_states_set) for e in smact_elems | ||
| ] | ||
| elif oxidation_states_set == "wiki": | ||
| warnings.warn( | ||
| "This set of oxidation states is sourced from Wikipedia. The results from using this set could be questionable and should not be used unless you know what you are doing and have inspected the oxidation states.", | ||
| stacklevel=2, | ||
| ) | ||
| ox_combos = [e.oxidation_states_wiki for e in smact_elems] | ||
|
|
||
| else: | ||
| raise ( | ||
| Exception( | ||
| f'{oxidation_states_set} is not valid. Enter either "default", "icsd", "pymatgen","wiki" or a filepath to a textfile of oxidation states.' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Refactor to reduce redundancy.
The code for handling different oxidation_states_set options is repetitive. Consider refactoring to improve maintainability and readability.
oxi_set = {
"default": [e.oxidation_states for e in smact_elems],
"icsd": [e.oxidation_states_icsd for e in smact_elems],
"pymatgen": [e.oxidation_states_sp for e in smact_elems],
"wiki": [e.oxidation_states_wiki for e in smact_elems],
}
if oxidation_states_set in oxi_set:
ox_combos = oxi_set[oxidation_states_set]
if oxidation_states_set == "wiki":
warnings.warn(
"This set of oxidation states is sourced from Wikipedia. The results from using this set could be questionable and should not be used unless you know what you are doing and have inspected the oxidation states.",
stacklevel=2,
)
elif os.path.exists(oxidation_states_set):
ox_combos = [
oxi_custom(e.symbol, oxidation_states_set) for e in smact_elems
]
else:
raise Exception(
f'{oxidation_states_set} is not valid. Enter either "default", "icsd", "pymatgen","wiki" or a filepath to a textfile of oxidation states.'
)Tools
GitHub Check: codecov/patch
[warning] 492-492: smact/screening.py#L492
Added line #L492 was not covered by tests
Pull Request Template
Description
smact_validityfunction.Fixes #281
Type of change
How Has This Been Tested?
Test Configuration:
Reviewers
N/A
Checklist:
Summary by CodeRabbit
New Features
smact_validityfunction to include anoxidation_states_setparameter for more flexible charge-balancing options.DopantPredictionTestto ensure robustness in dopant prediction features.Updates
README.mdto usehttps://instead ofgit://.numpyrequirement to<2.Documentation
"tabulate"module to imported modules list in documentation configuration.Bug Fixes
Doperclass with additional parameters and methods for better performance and accuracy.Tests
test_core.pyforsmact_validityfunction.