Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add oxidation states option to SMACT validity function #282

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Add oxidation states option to SMACT validity function #282

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b45603f
Add option to select oxidation states set for the smact_validity func…
AntObi Jul 8, 2024
daa9ff0
Fix docstring
AntObi Jul 8, 2024
7a7de52
Update docstring
AntObi Jul 8, 2024
226f41c
Pin numpy<2
AntObi Jul 8, 2024
a805bcf
Fix installation instructions
AntObi Jul 8, 2024
88019d9
Add new test for smact_validity
AntObi Jul 8, 2024
cbf7515
Fix docstring
AntObi Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion smact/screening.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,13 +430,19 @@
composition: Union[pymatgen.core.Composition, str],
use_pauling_test: bool = True,
include_alloys: bool = True,
oxidation_states_set: Union[str, bytes, os.PathLike] = "default",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

) -> 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py to cover different values of the oxidation_states_set parameter.
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


Returns:
bool: True if the composition is valid, False otherwise
Expand All @@ -462,7 +468,31 @@
space = element_dictionary(elem_symbols)
smact_elems = [e[1] for e in space.items()]
electronegs = [e.pauling_eneg for e in smact_elems]
ox_combos = [e.oxidation_states for e in smact_elems]

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 = [

Check warning on line 479 in smact/screening.py

View check run for this annotation

Codecov / codecov/patch

smact/screening.py#L474-L479 

Added lines #L474 - L479 were not covered by tests
oxi_custom(e.symbol, oxidation_states_set) for e in smact_elems
]
elif oxidation_states_set == "wiki":
warnings.warn(

Check warning on line 483 in smact/screening.py

View check run for this annotation

Codecov / codecov/patch

smact/screening.py#L482-L483 

Added lines #L482 - L483 were not covered by tests
"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]

Check warning on line 487 in smact/screening.py

View check run for this annotation

Codecov / codecov/patch

smact/screening.py#L487 

Added line #L487 was not covered by tests

else:
raise (

Check warning on line 490 in smact/screening.py

View check run for this annotation

Codecov / codecov/patch

smact/screening.py#L490 

Added line #L490 was not covered by tests
Exception(
f'{oxidation_states_set} is not valid. Enter either "default", "icsd", "pymatgen","wiki" or a filepath to a textfile of oxidation states.'
)
)
Comment on lines +474 to +496
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


threshold = np.max(count)
compositions = []
for ox_states in itertools.product(*ox_combos):
Expand Down