Corrections to NIST dataset #142
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes - Correct atomic mass unit conversion in the test cases dict. - Use pytest.approx to handle comparison of floating point numbers. - Recompile test files for this dataset and add dianion species to test cases.
PaulWAyers
approved these changes
Mar 25, 2025
Member
PaulWAyers
left a comment
PaulWAyers
left a comment
There was a problem hiding this comment.
It looks good to me. What do you think @marco-2023 or @msricher ?
Collaborator
|
I think using E(Ha) = E(cm⁻¹) / (CMINV cm⁻¹/Ha) is good, actually, it makes sense to me, unit-wise. Thanks for catching my mistake with the zero-electron species!!! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses the following issues related to the NIST dataset:
For example the energy for hydrogen should be about
this is caused by using$energy * CMINV$ when converting units. I've changed this to $energy / CMINV$ but alternatively we may want to redefine the CMINV variable.
Anionic species with charge -1 returning None value of energy.$E_{anion} = E_{neutral} - IP_{anion}$
This problem was raised in issue NIST dataset #100. As solution I used:
Incorrect variable being returned by the
Species.atmassattribute.This is not a specific problem of NIST dataset, but was caught by this dataset tests in PR Add zero-electron species to gaussian dataset #138 causing the checks to fail for python > v3.9. The attribute was returning the value from the .msg file instead of the one from the Element class instance in the periodic module.
This changes are a WIP, the following tasks remain to be completed: