-
Notifications
You must be signed in to change notification settings - Fork 14
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 optimal_geometry method #11
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.
Thank you for your PR! I have several comments, mostly minor and should be easily addressed. Overall, it looks good!
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
==========================================
+ Coverage 92.00% 92.09% +0.08%
==========================================
Files 72 72
Lines 7505 7564 +59
==========================================
+ Hits 6905 6966 +61
+ Misses 600 598 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I have pushed what I think are all the requested changes - please let me know if you have any more or if there is anything else you'd like to see changed before merge. |
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 for the changes. I have several minor comments on the changes like before.
Another thing is the mypy
test fails (you can see the red cross next to typestyle test). Can you fix this in this PR? If not, I can fix it after this being merged.
To test it locally, you can just execute in terminal mypy
in the same directory as the file mypy.ini
.
dqc/system/mol.py
Outdated
|
||
Arguments | ||
--------- | ||
kwargs |
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.
Please use **kwargs
here instead of kwargs
to differentiate between method(kwargs)
and method(**kwargs)
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.
done
I have now fixed all of those, please let me know if there is anything else or if you want things fixed in a different way.
I think it might be best if you fix the mypy errors if you can. I'm not so familiar with mypy and the best way to fix them. It looks like there are three
I have allowed edits from maintainers so feel free to directly push to this PR too |
Great, thanks for the PR! It looks good to me, so I will merge it once the ci tests are passed. I can do the mypy correction later. |
This PR closes #10 by adding an
optimal_geometry
method to the properties API. It also adds amake_copy
method to theMol
andSol
systems. It adds basic tests to cover themake_copy
andoptimal_geometry
.Overall I tried to match the code, naming, and commenting style of the repo as best I could - feel free to change or request changes on anything you don't like.
One thing I am wondering is if you still like the
optimal_geometry
name and function descriptions. Alternatives could emphasize that it is a position that is a local minimum of the energy. I don't really mind.