Unsure of purpose of piece of "get_el_sp" function in core.periodic_table

[benrich37]

The get_el_sp function in core.periodic_table is currently (as seen here)

`
@functools.lru_cache
def get_el_sp(obj: int | SpeciesLike) -> Element | Species | DummySpecies:
"""Utility function to get an Element, Species or DummySpecies from any input.

If obj is an Element or a Species, it is returned as is.
If obj is an int or a string representing an integer, the Element with the
atomic number obj is returned.
If obj is a string, Species parsing will be attempted (e.g. Mn2+). Failing that
Element parsing will be attempted (e.g. Mn). Failing that DummyElement parsing
will be attempted.

Args:
    obj (SpeciesLike): An arbitrary object. Supported objects are actual Element/Species,
        integers (representing atomic numbers) or strings (element symbols or species strings).

Raises:
    ValueError: if obj cannot be converted into an Element or Species.

Returns:
    Element | Species | DummySpecies: with a bias for the maximum number
        of properties that can be determined.
"""
# If obj is already an Element or Species, return as is
if isinstance(obj, Element | Species | DummySpecies):
    if getattr(obj, "_is_named_isotope", None):
        return Element(obj.name) if isinstance(obj, Element) else Species(str(obj))
    return obj

# If obj is an integer, return the Element with atomic number obj
try:
    flt = float(obj)
    assert flt == int(flt)  # noqa: S101
    return Element.from_Z(int(flt))
except (AssertionError, ValueError, TypeError, KeyError):
    pass

# If obj is a string, attempt to parse it as a Species
try:
    return Species.from_str(obj)  # type: ignore[arg-type]
except (ValueError, TypeError, KeyError):
    pass
# If Species parsing failed, try Element
try:
    return Element(obj)  # type: ignore[arg-type]
except (ValueError, TypeError, KeyError):
    pass

# If Element parsing failed, try DummySpecies
try:
    return DummySpecies.from_str(obj)  # type: ignore[arg-type]
except Exception as exc:
    raise ValueError(f"Can't parse Element or Species from {obj!r}") from exc`

but I am unsure specifically why returning Element(obj.name) if isinstance(obj, Element) else Species(str(obj)) is necessary for named isotopes, instead of returning obj. All the attributes (visible to my IDE) for the output of get_el_sp for a named isotope appear identical when running this function with and without the if getattr(obj, "_is_named_isotope", None) check commented out. Running all the tests on my personal machine with the if getattr(obj, "_is_named_isotope", None) check commented out also raised no errors

[DanielYang59]

I don't know the reason either, with:

if isinstance(obj, Element | Species | DummySpecies):
    return obj

I got no error too but the test coverage could be missing somewhere.

As per the docstring it should be returned "as is" (return obj):

pymatgen/src/pymatgen/core/periodic_table.py

Line 1628 in 31f1e1f

If obj is an Element or a Species, it is returned as is.

---

Looks like this check was added in #3542 1918eea, @esoteric-ephemera could you please comment on this? Thanks.

[DanielYang59]

Thanks I appreciate the context, make sense to me. Perhaps we should issue a warning in this (edge) case as we're converting D2O to H2O? (Inside if getattr(obj, "_is_named_isotope", False):)

Also looks like test is missing for this, would you mind if I add a test (and a comment) for this? Or would you like to get your hands on this yourself?

[esoteric-ephemera]

This isn't a priority for me, as it's already documented in the PR/tests and there are no reports of unexpected behaviors from people. Feel free to

[DanielYang59]

Really appreciate your reply, I just had another look and it appears this code block is not doing what we intended (feel free to point out if I misunderstood anything).

---

Pretty sure this is needed to convert isotopes to their parent representation (e.g., converting deuterium or tritium to hydrogen), and thereby have compositions like Composition("D2O") automatically convert to Composition("H2O").

I still don't really understand how this is achieved, as the name of an isotope is the "isotope name", for example "D" for Element.D/Species("D"), so if I understand correctly an Element/Species of isotope passed into get_el_sp would be returned as is:

pymatgen/src/pymatgen/core/periodic_table.py

Line 1649 in 31f1e1f

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

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


print(Element.D.name)  # D
print(Element.D.symbol)  # H

print(get_el_sp(Element.D).name)  # D
print(get_el_sp(Element.D) is 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 it seems we want the add_charges_from_oxi_state_guesses method to remove isotope characteristic:

pymatgen/tests/core/test_composition.py

Lines 845 to 851 in 31f1e1f

	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 I believe this is done not by:

pymatgen/src/pymatgen/core/periodic_table.py

Lines 1648 to 1649 in 31f1e1f

	if getattr(obj, "_is_named_isotope", False):
	return Element(obj.name) if isinstance(obj, Element) else Species(str(obj))

But the following call stack: add_charges_from_oxi_state_guesses -> _get_oxi_state_guesses -> get_el_amt_dict

As the el.symbol in L749 would convert an isotope to its "base" element ("H" for Element.D):

pymatgen/src/pymatgen/core/composition.py

Lines 741 to 750 in 31f1e1f

	def get_el_amt_dict(self) -> dict[str, float]:
	"""
	Returns:
	dict[str, float]: element symbol and (unreduced) amount. E.g.
	{"Fe": 4.0, "O": 6.0}.
	"""
	dct: dict[str, float] = defaultdict(float)
	for el, amt in self.items():
	dct[el.symbol] += amt
	return dict(dct)

---

In our case:

pymatgen/src/pymatgen/core/composition.py

Lines 963 to 975 in 31f1e1f

	_, oxidation_states = self._get_oxi_state_guesses(all_oxi_states, max_sites, oxi_states_override, target_charge)
	# Special case: No charged compound is possible
	if not oxidation_states:
	return type(self)({Species(e, 0): f for e, f in self.items()})
	# Generate the species
	species = []
	for el, charges in oxidation_states[0].items():
	species.extend([Species(el, c) for c in charges])
	# Return the new object
	return type(self)(collections.Counter(species))

oxidation_states (L963) would be ({'H': (1, 1), 'O': (-2,)},) which would be used to regenerated the Species in L972 (with isotope characteristic removed)

pymatgen/src/pymatgen/core/composition.py

Lines 971 to 972 in 31f1e1f

	for el, charges in oxidation_states[0].items():
	species.extend([Species(el, c) for c in charges])

---

This could also be confirmed by tests in #4193, i.e. with that code block removed, all tests still pass (including test_isotopes for Composition above).

[esoteric-ephemera]

OK - my guess is that I had to add the line to handle DummySpecies correctly. That would be the only case where a class is transmuted on output.

If I'm mistaken, then I see no harm removing the line

[DanielYang59]

Thanks for the input, can you elaborate on what case would break get_el_sp by DummySpecies and I would update the tests.

I tried the following and didn't see any exception yet (with if getattr(obj, "_is_named_isotope", False): commented out):

from pymatgen.core.periodic_table import get_el_sp, DummySpecies


print(get_el_sp(DummySpecies("X")))

print(get_el_sp(DummySpecies("X", 2, spin=3)))

print(get_el_sp(DummySpecies("Xg", 3)))

print(get_el_sp(DummySpecies.from_str("X")))

print(get_el_sp(DummySpecies.from_str("X2+")))

print(get_el_sp(DummySpecies.from_str("X2+spin=5")))

Which gives the following without error as they don't carry _is_named_isotope attribute so basically returned as is:

X0+
X2+,spin=3
Xg3+
X0+
X2+
X2+,spin=5.0

Is there any case we would need DummySpecies to carry _is_named_isotope maybe?