-
Notifications
You must be signed in to change notification settings - Fork 15
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
Support promolecules with single atom #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @msricher . I'd like to get @marco-2023 and/or @gabrielasd 's feedback too, but it looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to do the same trick for charges/mults in lines 582-592?
Yep you're right. I didn't think about both of those also being single values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @marco-2023 and/or @gabrielasd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me too.
@msricher could you add a test case for this fix to test_promolecule.py
It could be a slight change of this test case in the module:
AtomDB/atomdb/test/test_promolecule.py
Lines 83 to 88 in abf1008
pytest.param( | |
{ | |
"atnums": [4], | |
"charges": [1.2], | |
"mults": [1.2], | |
"coords": np.asarray( |
I don't think this is related to the changes Michelle introduced, but the CI checks are failing when building the website. It mentions an incompatibility between Scipy library and python version. This doesn't make sense to me since the rest of the checks for different python envs are passing. @msricher or @marco-2023 could you look into this? |
I am not familiar with the technical part, but the failing test is fixed now. |
We used to limit the top version of Scipy because we were using its old-fashioned generalized inverse. We don't use that anymore (and probably not at all in this package) so this is a good update on the dependencies. |
Fixes #134