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: fix filtering extra elements #606

Merged
merged 3 commits into from
May 24, 2022

Conversation

rkingsbury
Copy link
Contributor

Fixes bug in which get_pourbaix_entries was not properly filtering out extra elements (i.e., solid entries included in the internal phase diagram because they are reference solids for ions, but that are not part of the originally-supplied chemical system)

Contributor Checklist

  • I have run the tests locally and they passed.
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@rkingsbury
Copy link
Contributor Author

@mkhorton this PR fixes the bug you found related to element filtering. However, in the course of debugging I noticed that the energies of all Pourbaix entries returned by this package differ from those returned by the legeacy get_pourbaix_entries method in pymatgen. I added a test here to verify that the ion entries are being positioned correctly relative to the solid entries; the difference seems to arise from the solid entries themselves.

For example, the legacy client returns the following energy_per_atom

('mp-2414', 0.37806497411458206)
('mp-24118', 0.9102134379934201)

while the new client (in this package) returns

('mp-2414', 0.503819126302082)
('mp-24118', 0.9366894532565775)

It's unclear why this is happening, as all the arguments related to corrections, etc. are identical between the old and new clients.

This may be related to collection differences? Because If I just examine the solid entry using get_entry_by_material_id, I get

[('mp-2414', -6.448287344166668)]

from the legacy client and

[('mp-2414', -6.322537344166668)]

from the new client.

@rkingsbury
Copy link
Contributor Author

Also, all CI tests seem to be failing due to a missing API key:

>           raise MPRestError(
                f"REST query returned with error status code {response.status_code} "
                f"on URL {response.url} with message:\n{message}"
            )
E           mp_api.core.client.MPRestError: REST query returned with error status code 401 on URL https://api.materialsproject.org/xas/?_fields=edge&edge=K&_limit=1 with message:
E           Response {
E             "message":"No API key found in request"
E           }

src/mp_api/core/client.py:652: MPRestError

@munrojm
Copy link
Member

munrojm commented May 24, 2022

No API key is normal. If the tests pass locally, I can merge and it should be fine. Let me know when you are good to go.

Also, just an FYI that the difference in the energy for that material appears to be from the removal of an incorrect anion correction applied to sulfur in the new data.

@rkingsbury
Copy link
Contributor Author

Also, just an FYI that the difference in the energy for that material appears to be from the removal of an incorrect anion correction applied to sulfur in the new data.

OK, that makes sense. I was primarily looking at sulfur-containing systems so that must have been the origin of the discrepancy. I checked some other systems and the differences were smaller or non-existent. I'm good.

@munrojm munrojm added the release:patch Patch release label May 24, 2022
@munrojm munrojm merged commit efb161e into materialsproject:main May 24, 2022
@mkhorton
Copy link
Member

Thanks for taking a look and offering this fix @rkingsbury!

itertools.chain.from_iterable(i.elements for i in ion_ref_comps)
)
# TODO - would be great if the commented line below would work
# However for some reason you cannot process GibbsComputedStructureEntry with
# MaterialsProjectAqueousCompatibility
ion_ref_entries = self.get_entries_in_chemsys(
list(set([str(e) for e in ion_ref_elts] + ["O", "H"])),
list([str(e) for e in ion_ref_elts] + ["O", "H"]),
Copy link
Member

Choose a reason for hiding this comment

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

This list() is redundant

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

Successfully merging this pull request may close these issues.

3 participants