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

Remove unnecesary isotope conversion in core.periodic_table.get_el_sp #4193

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 23, 2024

To close #4171.

This isotope conversion may not be working as we expected as the name of an isotope Element/Species is the "isotope name" (e.g. "D" for Element.D and Species("D") instead of "H"):

return Element(obj.name) if isinstance(obj, Element) else Species(str(obj))

def __str__(self) -> str:
output = self.name if hasattr(self, "name") else self.symbol

from pymatgen.core.periodic_table import Element, Species, get_el_sp


print(get_el_sp(Element.D).name)  # D
print(get_el_sp(Element.D) == Element.D)  # True

print(get_el_sp(Species("D")).name)  # D
print(get_el_sp(Species("D")) == Species("D"))  # True

print(get_el_sp(Species("D", 1)).name)  # D
print(get_el_sp(Species("D", 1)) == Species("D", 1))  # True

Meanwhile the isotope characteristic removing in Composition.add_charges_from_oxi_state_guesses is not done by this but Composition.get_el_amt_dict, full analysis in #4171 (reply in thread).

This could also be confirmed by all unit tests passing including:

def test_isotopes(self):
composition = Composition({"D": 2, "O": 1})
assert "Deuterium" in [elem.long_name for elem in composition.elements]
# adding oxidation state removes Deuterium characteristic
composition = composition.add_charges_from_oxi_state_guesses()
assert "Deuterium" not in [elem.long_name for elem in composition.elements]
# however the user can explicitly add an oxidation state to deuterium
composition = Composition({"D+": 2, "O": 1})
assert composition.elements[0].oxi_state == 1
assert "Deuterium" in [elem.long_name for elem in composition.elements]

credit to @esoteric-ephemera for providing the context and the helpful discussion as always, and @benrich37 @mkhorton for bringing up the topic

@DanielYang59 DanielYang59 changed the title Add warning and unit test for isotope conversion in core.periodic_table.get_el_sp Remove unnecesary isotope conversion in core.periodic_table.get_el_sp Nov 23, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review November 23, 2024 07:59
@DanielYang59 DanielYang59 marked this pull request as draft November 24, 2024 03:17
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.

1 participant