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

Custom states #165

Merged
merged 10 commits into from
Nov 9, 2023
Merged

Custom states #165

merged 10 commits into from
Nov 9, 2023

Conversation

AntObi
Copy link
Collaborator

@AntObi AntObi commented Sep 5, 2023

Custom oxidation states call

Description

  • Enable users to load in their oxidation states text file
  • Allow smact_filter to use a custom oxidation state list
  • Fixed line-length to 80 in pre-commit-config.yaml for isort and black

Fixes #164

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Verified smact_filter works by passing an oxidation states text file to the oxidation_states_set argument

Test Configuration:

  • Python version: 3.9
  • Operating System: WSL2(Ubuntu)

Reviewers

@keeeto I've added the feature you've requested. I want to check if you're happy with the implementation before I merge it into the master branch

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@AntObi AntObi requested a review from keeeto September 5, 2023 14:34
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #165 (aea3937) into master (6ae0c2e) will increase coverage by 0.08%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   72.71%   72.80%   +0.08%     
==========================================
  Files          24       24              
  Lines        1913     1945      +32     
==========================================
+ Hits         1391     1416      +25     
- Misses        522      529       +7     
Files Coverage Δ
smact/screening.py 89.34% <100.00%> (+0.36%) ⬆️
smact/structure_prediction/database.py 86.84% <100.00%> (ø)
smact/structure_prediction/mutation.py 81.50% <ø> (ø)
smact/structure_prediction/prediction.py 44.18% <ø> (ø)
smact/structure_prediction/structure.py 70.14% <ø> (ø)
smact/tests/test_core.py 100.00% <100.00%> (ø)
smact/tests/test_structure.py 92.36% <100.00%> (ø)
smact/__init__.py 85.27% <80.00%> (-1.17%) ⬇️
smact/data_loader.py 74.00% <68.75%> (-0.34%) ⬇️

@mdi-group
Copy link

Hi @AntObi - I am just curious, where would one set the list of oxidation states to be used in a typical workflow?
For example in the SolarOxides example where would I tell it to use a different set of oxidation states?

@AntObi
Copy link
Collaborator Author

AntObi commented Nov 8, 2023

Hi @AntObi - I am just curious, where would one set the list of oxidation states to be used in a typical workflow? For example in the SolarOxides example where would I tell it to use a different set of oxidation states?

Currently to use the other built-in oxidation states list, we can modify that example as follows:

Original oxidation states call:

    # For each set of species (in oxidation states) apply both SMACT tests
    for ox_a, ox_b, ox_c in product(
        els[0].oxidation_states,
        els[1].oxidation_states,
        els[2].oxidation_states,
    ):

To use the built in icsd oxidation states:

    # For each set of species (in oxidation states) apply both SMACT tests
    for ox_a, ox_b, ox_c in product(
        els[0].oxidation_states_icsd,
        els[1].oxidation_states_icsd,
        els[2].oxidation_states_icsd,
    ):

Using the pymatgen structure prediction oxidation states in this example:

    # For each set of species (in oxidation states) apply both SMACT tests
    for ox_a, ox_b, ox_c in product(
        els[0].oxidation_states_sp,
        els[1].oxidation_states_sp,
        els[2].oxidation_states_sp,
    ):

I've originally made this pull request with the built-in smact_filter in mind, and a custom oxidation states file could be supplied to the oxidation_states_set argument.

If we want to have a similar API for making these calls, then I could make a new attribute for the Element class:
Element.oxidation_states_custom. For the examples above, we would access the attribute el.oxidation_states_custom. To implement this, the Element class can take an optional argument, let's call it oxi_states_custom_filepath, which will then set the property oxidation_states_custom to the oxidation states for that particular element based on the supplied file.

@mdi-group
Copy link

Thanks @AntObi - that is very clear. I understand now. I was struggling to see how this would be accessed. But I think that implementing in the smact_filter in the first instance is a sound decision. Code looks good and tests are passing. I think we can merge!

@keeeto
Copy link
Member

keeeto commented Nov 9, 2023

Hi @AntObi sorry I just realised I was commenting from the wrong account! Anyway, there appears to be one conflict to resolve then it can be merged.

@AntObi
Copy link
Collaborator Author

AntObi commented Nov 9, 2023

Hi @keeeto, I've made some more changes:

The Element class now takes an optional argument of oxi_states_custom_filepath which allows a user to supply a text file containing oxidation states. Then the oxidation states can be accessed through the attribute oxidation_states_custom.

Example:

import smact

Fe = smact.Element("Fe", oxi_states_custom_filepath = "/path/to/oxidation_states.txt")

print(Fe.oxidation_states_custom)

I've also updated the element_dictionary function to take the optional argument of oxi_states_custom_filepath. This should allow users to quickly create a list of Element objects which have access to the custom oxidation states.

So for the solar oxides example, you would apply the following changes:

Instead of

all_el = smact.element_dictionary()

You would create the dictionary of all elements with a supplied oxidation states text file:

all_el = smact.element_dictionary(oxi_states_custom_filepath = "/path/to/oxidation_states.txt")

Then the oxidation states call in the smact_filter function defined in that particular example becomes:

 # For each set of species (in oxidation states) apply both SMACT tests
    for ox_a, ox_b, ox_c in product(
        els[0].oxidation_states.custom,
        els[1].oxidation_states.custom,
        els[2].oxidation_states.custom,
    ):

Copy link
Member

@keeeto keeeto left a comment

Choose a reason for hiding this comment

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

All looks good to me!

@keeeto keeeto merged commit 2586e16 into master Nov 9, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom oxidation state calls
3 participants