Skip to content

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

Merged
shyuep merged 18 commits intomaterialsproject:masterfrom
DanielYang59:element-skip-isotope
Jan 9, 2025
Merged

[Breaking/Fix] Skip isotopes when iterating through core.Element#4180
shyuep merged 18 commits intomaterialsproject:masterfrom
DanielYang59:element-skip-isotope

Conversation

@DanielYang59
Copy link
Copy Markdown
Contributor

@DanielYang59 DanielYang59 commented Nov 17, 2024 •
edited
Loading

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
Copy Markdown
Member

shyuep commented Nov 18, 2024

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

@esoteric-ephemera
Copy link
Copy Markdown
Contributor

esoteric-ephemera commented Nov 18, 2024

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
Copy Markdown
Contributor Author

DanielYang59 commented Nov 19, 2024 •
edited
Loading

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
Copy Markdown
Contributor Author

DanielYang59 commented Nov 19, 2024 •
edited
Loading

@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
@shyuep shyuep merged commit 27230b1 into materialsproject:master Jan 9, 2025
@DanielYang59 DanielYang59 deleted the element-skip-isotope branch January 10, 2025 02:27
@esoteric-ephemera esoteric-ephemera mentioned this pull request Jul 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DanielYang59 @shyuep @esoteric-ephemera