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

Include some special casing for chemical species #126

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

Conversation

mfherbst
Copy link
Member

Adds a few more special cases to the handling of chemical species plus a bit of cleanup.

:X is fairly common in atomic structure files as a "dummy atom" with atomic number 0. We can either force everyone to drop such atoms before using our stuff or include support here.

Here I am proposing to add this special case along with :T for tritium.

Note that I'm changing the Z -> data cache from an array to a Dict. Not sure if that has big performance implications, one could also handle this differently. @cortner probably knows if this may be an issue.

@cortner
Copy link
Member

cortner commented Oct 25, 2024

I suggest to discuss any further changes to chemical species with @tjjarvinen -- there is a definite need to overhaul this and I understand he has a concrete plan.

@cortner
Copy link
Member

cortner commented Oct 25, 2024

Note that I'm changing the Z -> data cache from an array to a Dict. Not sure if that has big performance implications, one could also handle this differently. @cortner probably knows if this may be an issue.

I doubt this matters in practice.

@cortner
Copy link
Member

cortner commented Oct 25, 2024

I don't mind about :T and :X, I have no strong views. I do suggest though that any atoms of type :X have no units...

@tjjarvinen
Copy link
Collaborator

Yes, I have plan and can make PR over weekend. After that we can discuss how to proceed.

@mfherbst
Copy link
Member Author

Ok sounds good.

@mfherbst
Copy link
Member Author

mfherbst commented Nov 2, 2024

What's the status with respect to the overhaul of ChemicalSpecies?

The inclusion of 0 / :Z is blocking for me in the move to AtomsBase 0.4. If redesigning ChemicalSpecies will take some time (fine for me), could we get this minimal modification merged ?

@tjjarvinen
Copy link
Collaborator

I was busy moving to back Europe the last week. The new version only needs masses added for isotopes and few other small things. I should be able to do it by tomorrow.

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