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

How to improve Composition.__repr__ #3181

Closed
janosh opened this issue Jul 25, 2023 · 6 comments · Fixed by #3182
Closed

How to improve Composition.__repr__ #3181

janosh opened this issue Jul 25, 2023 · 6 comments · Fixed by #3182
Labels
core Pymatgen core discussion ux User experience

Comments

@janosh
Copy link
Member

janosh commented Jul 25, 2023

There's a problem with Composition.__repr__.

from pymatgen.core import Composition
from pymatgen.core.periodic_table import Species

comp1 = Composition({Species("Fe2+"): 2})
comp2 = Composition({"Fe2+": 2})
comp3 = Composition({"Fe": 2})

These 3 compositions all print

Comp: Fe2

which isn't ideal given only the first 2 are equal.

comp1 == comp2
>>> True
comp1 == comp3
>>> False

Ideally, the output of Composition.__repr__ should allow to recreate the Composition by pasting that string into the Python interpreter. Current __repr__ is lacking oxi_state information and also doesn't produce valid Python code.

Suggestion

How about we change to:

print(f"{Composition('Fe2+:2')!r}")
>>> Composition('Fe2+:2')

This would strictly speaking be a breaking change since people might have written code that depends on how Composition stringifies. Curious to hear opinion @mkhorton @shyuep.

@janosh janosh added discussion ux User experience core Pymatgen core labels Jul 25, 2023
@janosh
Copy link
Member Author

janosh commented Jul 25, 2023

Another example mp-18767:

Composition('Li+:2 Mn3+:2 O2-:4')

Before

Comp: Li2 Mn2 O4

@chiang-yuan
Copy link
Contributor

This is a good proposal, but what if in the future we consider even more detailed information like partial charge, would it be more complicated? I guess we can just round the partial charge to formal charge and use formal charge as species tag?

@janosh
Copy link
Member Author

janosh commented Jul 25, 2023

For added background, the plan is to keep the current __str__ method. In Python str and repr serve 2 different purposes:

  • repr goal is to be unambiguous
  • str goal is to be readable

repr is more for devs and str more for users.

@Andrew-S-Rosen
Copy link
Member

Yup, retracted my comment! :)

@shyuep
Copy link
Member

shyuep commented Jul 25, 2023

I think changing repr is fine. No one should be using repr for any purposes.

@janosh
Copy link
Member Author

janosh commented Jul 25, 2023

Cool. @tschaume @yang-ruoxi any downstream implications for mp-web / crystaltoolkit here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pymatgen core discussion ux User experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants