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

InsertionElectrode bug fix & doc clarification #2257

Merged
merged 12 commits into from
Nov 16, 2021

Conversation

acrutt
Copy link
Contributor

@acrutt acrutt commented Oct 8, 2021

The InsertionElectrode.from_entries() method can use either ComputedEntries or ComputedStructureEntries - this has been added to the documentation. Setting strip_structures=True only works with ComputedStructureEntries so this has also been clarified.

In constructing the PhaseDiagram to compare the energies of the provided battery entries, the element energies were not always set high enough if there was a large energy difference between the empty host material and intercalated material. As a result, no intercalated entry was found to be stable and voltage pairs could not be formed. The factor for increasing the element_energy was set higher to 1e9 to avoid this problem.

@acrutt acrutt changed the title InsertionElectrode bug fix & doc clarification [WIP] InsertionElectrode bug fix & doc clarification Oct 14, 2021
@coveralls
Copy link

coveralls commented Nov 11, 2021

Coverage Status

Coverage decreased (-0.6%) to 83.111% when pulling 59f1b2e on acrutt:master into 9276567 on materialsproject:master.

@acrutt
Copy link
Contributor Author

acrutt commented Nov 15, 2021

Note for future reference on the appropriate value for the terminal element entries when constructing the phase diagram. This value cannot be set infinitely high otherwise there will be precision errors in constructing the ConvexHull.

http://www.qhull.org/html/qh-impre.html

Your input coordinates start with the same five or more digits (i.e., it is shifted relative to the origin). This reduces the available precision.

Therefore the terminal elements need to have an energy comparable to the provided battery material energies. 10 can be an appropriate value while 1e9 is too big. If the value is set too big (values greater than 1e6), pymatgen's tests will fail due to incorrect convex hull construction that results in misidentification of the stable battery entries.

@mkhorton
Copy link
Member

Thanks @acrutt and for providing additional context here

@acrutt acrutt changed the title [WIP] InsertionElectrode bug fix & doc clarification InsertionElectrode bug fix & doc clarification Nov 16, 2021
@acrutt
Copy link
Contributor Author

acrutt commented Nov 16, 2021

@mkhorton I believe this PR is ready to merge. Ultimately the changes are now limited to some documentation clarifications. Let me know if any other work is needed!

@mkhorton mkhorton merged commit e084ec1 into materialsproject:master Nov 16, 2021
@mkhorton
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants