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

[Breaking/Fix] Skip isotopes when iterating through core.Element #4180

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 17, 2024

Summary

EnumType is responsible for setting the correct repr(), str(), format(), and reduce() methods on the final enum, as well as creating the enum members, properly handling duplicates, providing iteration over the enum class, etc.

  • Add property Element.named_isotopes for all named_isotopes so far.
  • Clean up core.periodic_table docstring (remove non-existent PeriodicTable class).

This would be a breaking change (also might be called a fix), so comments are hugely appreciated.


from pymatgen.core import Element

for idx, ele in enumerate(Element):
    print(idx, ele)

Before

0 H
1 H  # Deuterium
2 H  # Tritium
3 He
4 Li
5 Be
6 B
7 C
......

Now (nothing special, just skip isotopes of hydrogen)

0 H
1 He
2 Li
3 Be
4 B
5 C
......

@DanielYang59 DanielYang59 changed the title Skip isotopes when iterating through core.Element [Breaking/Fix] Skip isotopes when iterating through core.Element Nov 17, 2024
@shyuep
Copy link
Member

shyuep commented Nov 18, 2024

I agree isotopes shouldn't appear as part of default behavior.

@esoteric-ephemera
Copy link
Contributor

That was an oversight on my part, thanks for the fix! Also agree that enumerating over elements shouldn't list isotopes. Maybe adding a method to Element to show people which isotopes are available would be useful tho?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Nov 19, 2024

That was an oversight on my part, thanks for the fix!

No problem at all, cannot blame anyone, it's the test that is missing, together we improve test coverage :)

Maybe adding a method to Element to show people which isotopes are available would be useful tho?

Fair point, is current implementation looking good to you @esoteric-ephemera (I don't have much experience overwriting an Enum so not sure if there's any pitfall like builtin dict/list/..., do review with a pinch of salt)?

Element.named_isotopes # ---> (Element.D, Element.T)

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Nov 19, 2024

@janosh I believe you're very experienced with Enum (while I'm not), perhaps you could help me double-check the implementation?

@DanielYang59 DanielYang59 marked this pull request as ready for review November 21, 2024 03:22
@DanielYang59 DanielYang59 marked this pull request as draft December 11, 2024 15:29
@DanielYang59 DanielYang59 marked this pull request as ready for review December 12, 2024 04:46
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.

3 participants