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

Add multiplicicites table for verification of compilation scripts #36

Merged
merged 16 commits into from
Mar 22, 2024

Conversation

gabrielasd
Copy link
Collaborator

We needed to verify the input multiplicity parameter in the compilation scripts to make sure the requested species matched the one in the available raw data.

This verification is being done differently for every dataset (if done at all). The better approach to do this check was the one implemented in the nist dataset, where the multiplicity of the neutral and cationic species was taken from the file database_beta_1.3.0.h5 and for the anions the multiplicity of the isoelectronic species was read from the MULTIPLICITIES table in atomdb.api.

This PR adds a table of multiplicities for elements from Hydrogen (Z=1) to Fermium (Z=100) with charges ranging from -2 to Z-1. The values in the table were populated from database_beta_1.3.0.h5 following the procedure in nist dataset.
There is also an utility function to read the value of multiplicity from this table, given the atomic number and charge.
Then, this could be use as the common way to make the verification in all compilation scripts.

@gabrielasd gabrielasd requested a review from msricher March 11, 2024 16:35
@gabrielasd
Copy link
Collaborator Author

I noticed that PR #32, like this one, also has a correct solution for issue #11.
However, I think the solution here is a bit more general, at the expense of introducing an extra data file (multiplicities_table.scv)

Copy link
Collaborator

@msricher msricher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR still necessary? If so, I think it's a good start.

In data/utils.py, I would just instantiate a single MultsTable instance, since I think it's a singleton pattern; then export the instance.

__all__ = [
    "multiplicities",
    ...
]

class _MultsTable:
   ...

multiplicities = _MultsTable()

Better yet, if this can just be a function that captures some state from variables defined at the top-level in the file, then I'd do that. Only use singletons when really necessary. Not a big deal though.

Finally, I would not hard-code the range of charges (+whatever to -2). That should be a variable (maybe a constant, or an argument to MultsTable.__init__().

@gabrielasd
Copy link
Collaborator Author

Thanks @msricher those are good points. I'll make the fix so the instance get exported.

There is room for improvement for sure, still, I'd suggest merging this for now, so that at least for the compilation scripts it can be used to make sure we are generating the right species.
For the promolecule module something better can be implemented latter.

@msricher
Copy link
Collaborator

Ok, that works for me!

@gabrielasd
Copy link
Collaborator Author

In the last commit I got rid of the MultsTable class in favour of a function that parses the table of multiplicities and dumps it as a dictionary.
Like @msricher suggested, I initialize the data so that it is available and can be imported by other modules.
Once this is merged it will solve issue #11

@marco-2023 marco-2023 merged commit 3d8f9d4 into master Mar 22, 2024
3 of 9 checks passed
@marco-2023 marco-2023 deleted the fix_mult_numeric branch March 22, 2024 21:49
msricher pushed a commit that referenced this pull request May 13, 2024
Add multiplicicites table for verification of compilation scripts
msricher pushed a commit that referenced this pull request May 13, 2024
Add multiplicicites table for verification of compilation scripts
marco-2023 added a commit that referenced this pull request May 13, 2024
Add multiplicicites table for verification of compilation scripts
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