Bug in core.composition comparison

[DanielYang59]

Bug in core.composition comparison

Open this in case it got forgotten.

As found out by @janosh in #3792 (comment), there is an obvious bug with Composition comparison where True is returned too early:

pymatgen/pymatgen/core/composition.py

Lines 195 to 212 in 6c696cd

	def __ge__(self, other: object) -> bool:
	"""Composition greater than or equal to. We consider compositions A >= B
	if all elements in B are in A and the amount of each element in A is
	greater than or equal to the amount of the element in B within
	Composition.amount_tolerance.
	Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect).
	"""
	if not isinstance(other, Composition):
	return NotImplemented
	for el in sorted(set(self.elements + other.elements)):
	if other[el] - self[el] >= Composition.amount_tolerance:
	return False
	# TODO @janosh 2024-04-29: is this a bug? why would we return True early?
	if self[el] - other[el] >= Composition.amount_tolerance:
	return True
	return True

On top of this obvious bug, there are a few questions:

• The docstring adds to ambiguity: Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect)..
• It appears that we don't need to sort this union set?

How to define the comparison behavior

How should we define the behavior when an Element is only in one of the Composition and this Composition is not a superset of another? For example:

1. Composition({"O": 1, "N": 1}) > Composition({"O": 1}) # This makes sense
2. Composition({"N": 1}) > Composition({"O": 1}) # This feels a bit weird though the atomic number is indeed greater
3. Composition({"O": 2, "N": 1}) > Composition({"O": 2, "C": 1}) # This doesn't seem to make sense? But this could be inferred from (2)

So should we prevent Composition with different Element from being compared altogether?

pymatgen/tests/core/test_composition.py

Lines 504 to 516 in 6c696cd

	def test_comparisons(self):
	c1 = Composition({"S": 1})
	c1_1 = Composition({"S": 1.00000000000001})
	c2 = Composition({"S": 2})
	c3 = Composition({"O": 1})
	c4 = Composition({"O": 1, "S": 1})
	assert not c1 > c2
	assert not c1_1 > c1
	assert not c1_1 < c1
	assert c1 > c3
	assert c3 < c1
	assert c4 > c1
	assert sorted([c1, c1_1, c2, c4, c3]) == [c3, c1, c1_1, c4, c2]