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

Permitting Electrodes with Extractable Ions #483

Merged
merged 2 commits into from Jul 27, 2022

Conversation

acrutt
Copy link
Contributor

@acrutt acrutt commented Jul 27, 2022

The previous StructureGroupBuilder base query did not permit the processing of structures that contained ["Li", "Be", "Na", "Mg", "K", "Ca", "Rb", "Sr", "Cs", "Ba"]. However, it is possible to have insertion electrodes that contain extractable ions (e.g. ["H", "Li", "Na", "K", "Rb", "Cs", "Ag", "Cu"]) and these types of systems were being excluded from the InsertionElectrodeBuilder.

This requirement has been removed from the StructureGroupBuilder to allow the analysis of Insertion Electrodes with extractable ions. Note this is a significant change because the Insertion Electrodes will no longer be limited to systems that only contain a single mobile species, however all analysis will still only assume one species as the working ion.

…ng ions. Previously WORKING_IONS appeared to be a list of 1+ and 2+ ions that was poorly named.
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #483 (f6e8a97) into main (b7b458e) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
- Coverage   97.63%   97.56%   -0.07%     
==========================================
  Files         115       91      -24     
  Lines       23900    21830    -2070     
==========================================
- Hits        23334    21299    -2035     
+ Misses        566      531      -35     
Impacted Files Coverage Δ
emmet-core/emmet/core/utils.py 57.69% <0.00%> (-32.70%) ⬇️
emmet-core/emmet/core/oxidation_states.py 73.33% <0.00%> (-17.78%) ⬇️
emmet-core/emmet/core/qchem/molecule.py 80.18% <0.00%> (-16.99%) ⬇️
emmet-core/emmet/core/summary.py 80.00% <0.00%> (-10.38%) ⬇️
emmet-core/emmet/core/settings.py 88.52% <0.00%> (-9.84%) ⬇️
emmet-core/emmet/core/vasp/calc_types/utils.py 82.45% <0.00%> (-7.02%) ⬇️
emmet-core/emmet/core/molecules/atomic.py 93.15% <0.00%> (-2.74%) ⬇️
emmet-core/emmet/core/mpid.py 94.44% <0.00%> (-1.86%) ⬇️
emmet-core/emmet/core/molecules/bonds.py 80.72% <0.00%> (-1.81%) ⬇️
emmet-core/emmet/core/elasticity.py 84.09% <0.00%> (-1.37%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 857811e...f6e8a97. Read the comment docs.

@munrojm
Copy link
Member

munrojm commented Jul 27, 2022

@acrutt Since the builders are currently using ["Ca", "Cs", "Rb", "K", "Y", "Na", "Al", "Mg", "Li", "Zn"] as the list of working ions, we should probably settle on the final list before I merge this.

@munrojm munrojm added the release:patch Patch updates label Jul 27, 2022
@munrojm munrojm merged commit 243fd27 into materialsproject:main Jul 27, 2022
@acrutt acrutt deleted the extractable_ions branch July 28, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:patch Patch updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants