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

Merge ElementBase into Element? #2999

Closed
janosh opened this issue May 18, 2023 · 5 comments
Closed

Merge ElementBase into Element? #2999

janosh opened this issue May 18, 2023 · 5 comments
Labels
breaking Breaking change needs discussion Needs discussion to agree on actionable next steps

Comments

@janosh
Copy link
Member

janosh commented May 18, 2023

@shyuep Should we merge ElementBase into Element? It says ElementBase has no enum values so it can be subclassed but it isn't actually subclassed anywhere in pymatgen. Seems cleaner to only have an Element class.

class ElementBase(Enum):
"""Element class defined without any enum values so it can be subclassed."""
def __init__(self, symbol: str):

@janosh janosh added needs discussion Needs discussion to agree on actionable next steps breaking Breaking change labels May 18, 2023
@mkhorton
Copy link
Member

Why was ElementBase created to start with, presumably for good reason? I vaguely remember this, can try and find the PR.

@janosh
Copy link
Member Author

janosh commented May 18, 2023

Here's the PR: #2005 by @jmmshn.

It also lists subclasses as the reason for creating ElementBase.

@mkhorton
Copy link
Member

I see the mention for API use. Perhaps it was used to define a class for intercalant elements?

@jmmshn
Copy link
Contributor

jmmshn commented May 18, 2023

I think it was to get nested as_dict / from_dict to work properly. So the problem was that all the classes in emmet that had these classes required very custom construction and I wanted to have a a definition of these classes that behaved more like dataclasses so serialization was less troublesome. There were many times where the objects serialized only when they were top level.

@janosh
Copy link
Member Author

janosh commented May 18, 2023

@jmmshn Thanks for the context! I'll add this to the ElementBase doc str.

@janosh janosh closed this as completed in ea3d300 May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change needs discussion Needs discussion to agree on actionable next steps
Projects
None yet
Development

No branches or pull requests

3 participants