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

get_ion_reference_data_for_chemsys: fix bug in str chemsys #756

Merged
merged 5 commits into from
May 15, 2023

Conversation

rkingsbury
Copy link
Contributor

MPRester.get_ion_reference_data_for_chemsys() does not always correctly return data when a chemsys is supplied as a str. The problem occurs because some element symbols are a subset of others, e.g. B is a subset of Bi. The code for this method currently tests whether any of the major elements associated with an ion are in chemsys:

return [d for d in ion_data if d["data"]["MajElements"] in chemsys]

Which causes problems because "B" is in "Bi`. For example:

>>> ion_data = m.get_ion_reference_data_for_chemsys(["Bi"])
>>> len(ion_data)
8
>>> ion_data = m.get_ion_reference_data_for_chemsys(["V"])
>>> len(ion_data)
14
>>> ion_data = m.get_ion_reference_data_for_chemsys("Bi-V")
>>> len(ion_data)
32

Passing the elements as a list is OK

>>> ion_data = m.get_ion_reference_data_for_chemsys("Bi-V")
>>> len(ion_data)
22

This PR should fix the problem by convert str chemsys into list always

@rkingsbury
Copy link
Contributor Author

@jmunro tests seem to be failing due to a missing API KEY in the CI. The test in question is marked with pytest.skipif and should not run when there's no API key, but I guess that isn't working?

E mp_api.client.core.client.MPRestError: REST query returned with error status code 401 on URL https://api.materialsproject.org/materials/bonds/?_all_fields=True&material_ids=mp-149&_limit=1 with message:
E Response {
E "message":"No API key found in request"
E }

Let me know if there's something I can do to help resolve - I'd like to get this bugfix merged soon to prevent subtle mistakes or confusion when people use the Pourbaix tools.

@munrojm munrojm added the release:patch Patch release label May 15, 2023
@munrojm munrojm merged commit 3ffecd2 into materialsproject:main May 15, 2023
@munrojm
Copy link
Member

munrojm commented May 15, 2023

Yup, planned to get back to this after making some other changes. Happy to merge now! Thanks for the reminder.

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.

2 participants