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

Remove unit module; use scipy.constants #33

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

msricher
Copy link
Collaborator

@msricher msricher commented Mar 8, 2024

I removed the units module and used scipy.constants as per #17.

I also did unit conversions to atomic units in the NIST dataset @gabrielasd.

@msricher msricher requested a review from gabrielasd March 8, 2024 16:16
@gabrielasd
Copy link
Collaborator

Looks good to me. Thanks @msricher
These changes guarantee consistency in the unit format in which we store the data, and the conversions are taken from Scipy, which were the main requirements of the issue.

For the future, as enhancement, I'd suggest keeping the units module as a place to gather all conversion related variables. For example there could be a variable:
ev = constants.eV / (2 * constants.Rydberg * constants.h * constants.c)
like we have for amu and Angstrom.
Likely this should go together with cleaning up the compilation scripts like Michelle has pointed out, since in my opinion is something necessary for the sake of readability of the code.

@gabrielasd gabrielasd closed this Mar 9, 2024
@gabrielasd gabrielasd reopened this Mar 9, 2024
@gabrielasd gabrielasd merged commit 968ebf1 into theochem:master Mar 9, 2024
16 of 17 checks passed
msricher pushed a commit that referenced this pull request May 13, 2024
Remove unit module; use scipy.constants
msricher pushed a commit that referenced this pull request May 13, 2024
Remove unit module; use scipy.constants
marco-2023 pushed a commit that referenced this pull request May 13, 2024
Remove unit module; use scipy.constants
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.

2 participants