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

Pourbaix: default to filter_solids=True #2177

Merged
merged 6 commits into from
Jun 20, 2021

Conversation

rkingsbury
Copy link
Contributor

Summary

Default PourbaixDiagram to use filter_solids=True for consistency with the MP website and discussion in #2173.

In addition:

  • Fix a bug in MaterialsProjectAqueousCompatibility introduced by e8028b0
  • add type hinting to PourbaixDiagram and make a few type hinting fixes to Entry classes (necessary to make mypy happy on PourbaixDiagram).
  • Deprecate the include_unprocessed_entries kwarg to PourbaixDiagram.as_dict. This was necessary in order to make serialization work properly after switching to filter_solids=True by default. To me, it appears this kwarg was a sort of workaround to allow a user to effectively decide whether to filter solids at the serialization / deserialization stage. In my mind, it makes more sense for the user to make that decision via the filter_solids kwarg when the diagram is constructed, then serialize the filter_solids kwarg along with all unprocessed entries always. I feel this is more consistent with the way I "expect" serialization to work. @montoyjh please comment if you have any concerns or had a specific reason for keeping this is a kwarg to as_dict

Closes #2173

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
rkingsbury referenced this pull request Jun 19, 2021
@montoyjh
Copy link
Contributor

I think it was more intended to include single entries that one might want to inspect and use for analysis post serial/deserialization in the multi-entry case, but I think it's probably overly complex and misnamed, so I'm in support of getting rid of it.

@mkhorton mkhorton merged commit e002483 into materialsproject:master Jun 20, 2021
@mkhorton
Copy link
Member

Thanks @rkingsbury, @shyamd, including discussion in e8028b0#commitcomment-52383083

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.

Significantly different Pourbaix diagrams somewhere between 2020.3.13 and 2022.0.8
3 participants