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

Bugs in Composition().__ge__() method #4204

Open
txy159 opened this issue Nov 28, 2024 · 1 comment
Open

Bugs in Composition().__ge__() method #4204

txy159 opened this issue Nov 28, 2024 · 1 comment
Labels

Comments

@txy159
Copy link

txy159 commented Nov 28, 2024

Python version

Python 3.9

Pymatgen version

2024.3.1

Operating system version

macos

Current behavior

image

Expected Behavior

The second one should output False.

Minimal example

from pymatgen.core import Composition

Composition("LiFePO") >= Composition("LiFeP")

Composition("LiFePO") >= Composition("LiFeH")

Relevant files to reproduce this bug

No response

@txy159 txy159 added the bug label Nov 28, 2024
@DanielYang59
Copy link
Contributor

DanielYang59 commented Nov 28, 2024

Admittedly the behaviour is surprising, but I believe this is expected, I had the same question in #3895

This comparison operation is not defined to really "compare" Composition (it might not make chemical sense to order compositions in the first place), but more to provide a stable ordering reference (such that when you sort compositions, they don't came out in a unpredictable/random pattern).

def __ge__(self, other: object) -> bool:
"""Composition greater than or equal to. We sort the elements in the compositions in order of
electronegativity. The amount of the most electropositive element that is not equal within a certain
tolerance factor is used to make a comparison. Note that an element not present in a Composition has an implied
amount of 0.
Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect).
"""

Note the elements are sorted by electronegativity, in this case the comparing order is [Element Li, Element Fe, Element P, Element H, Element O].

For the first comparison, the oxygen in "LiFeP" has an implied amount of zero (LiFePO0), it's therefore considered "smaller" than "LiFePO".

For the second case, the amount of P is greater in "LiFePO" than "LiFeH" (effectively LiFePH0O vs LiFeP0HO0) and thus the short-circuit:

for el in sorted(set(self.elements + other.elements)):
if other[el] - self[el] >= type(self).amount_tolerance:
return False
if self[el] - other[el] >= type(self).amount_tolerance:
return True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants