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.

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

Add oxidation states option to SMACT validity function #282

Merged
merged 7 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ conda install -c conda-forge smact

Alternatively, the very latest version can be installed using:

pip install git+git://github.com/WMD-group/SMACT.git
pip install git+https://github.com/WMD-group/SMACT.git

For developer installation SMACT can be installed from a copy of the source
repository (https://github.com/wmd-group/smact); this will be preferred if using experimental code branches.
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
test_suite="smact.tests.test",
install_requires=[
"scipy",
"numpy",
"numpy<2",
"spglib",
"pymatgen>=2024.2.20",
"ase",
Expand Down
38 changes: 35 additions & 3 deletions smact/screening.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,13 +430,21 @@
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.

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.
Comment on lines +433 to +447
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 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?


Returns:
bool: True if the composition is valid, False otherwise
Expand All @@ -462,7 +470,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 = [
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 (

Check warning on line 492 in smact/screening.py

View check run for this annotation

Codecov / codecov/patch

smact/screening.py#L492

Added line #L492 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
19 changes: 19 additions & 0 deletions smact/tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,25 @@ def test_smact_validity(self):
# Test for single element
self.assertTrue(smact.screening.smact_validity("Al"))

# Test for MgB2 which is invalid for the default oxi states but valid for the icsd states
self.assertFalse(smact.screening.smact_validity("MgB2"))
self.assertTrue(
smact.screening.smact_validity("MgB2", oxidation_states_set="icsd")
)
self.assertFalse(
smact.screening.smact_validity(
"MgB2", oxidation_states_set="pymatgen"
)
)
self.assertTrue(
smact.screening.smact_validity("MgB2", oxidation_states_set="wiki")
)
self.assertFalse(
smact.screening.smact_validity(
"MgB2", oxidation_states_set=TEST_OX_STATES
)
)

# ---------------- Lattice ----------------
def test_Lattice_class(self):
site_A = smact.lattice.Site([0, 0, 0], -1)
Expand Down