materialsproject / api

New API client for the Materials Project
https://materialsproject.github.io/api/
Other
107 stars 39 forks source link

get_ion_reference_data_for_chemsys: fix bug in str chemsys #756

Closed rkingsbury closed 1 year ago

rkingsbury commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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