-
Notifications
You must be signed in to change notification settings - Fork 79
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 torsiondrive compute procedure #305
Conversation
Looks great @SimonBoothroyd! Thank you for putting this together! I think this covers all the bases, and I prefer the simplicity of running one optimization at a time with I think we should press forward with this. Would you like to create a test next? |
Thanks @dotsdl - tests added in 4a68fe9. It looks like the failures are mostly un-related. I'm not sure why |
Agree with you @SimonBoothroyd that the errors we see in the tests are unrelated to the changes introduced by this PR. I wouldn't consider those blocking for merge of this feature. @loriab, is there anything additional you would like to see before merging this? This functionality simplifies execution of torsiondrives by QC* users substantially, and I'm eager to get it into the library! |
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.
This looks like a great start. It'd be good to have a torsiondrive procedure in QCEngine
- as a model of what duplicate qcng procedure and qcf service looks like
- because I plan to be migrating several more procedures into qcng in the next months and the more variety the less likely we'll make wrong/restrictive decisions
- there's a lot of experience running torsiondrive so easier to head off wrong decisions on multispawn procedures
lmk if a zoom-like mtg would be useful, and thanks for the new incipient functionality!
Thanks @loriab - I've made the changes you suggested and have opened up an initial PR for adding the models to QCElemental here MolSSI/QCElemental#268 where we can maybe discuss them in a bit more detail. |
qcel 0.22 ready with the models this PR needs |
Performing final review with aim to 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.
Looks great @SimonBoothroyd; incredible work! I have one question to address, but also ready to merge as-is!
Merged! Thank you @SimonBoothroyd for this! |
Description
This PR implements an initial proof of concept
torsiondrive
procedure as outlined in issue #306.There are a number of questions about this PR:
As
qcelemental
does not have native models for torsion drives I created some here based off of the QCElemental optimisation models and the QCPortal torsion drive models. Are these something that could / should live in QCElemental?Is it more efficient to run multiple torsion drives at once, each with access to one core, or one torsion drive at once using all of the cores.
If the QCE maintainers would be happy to accept a PR similar to this I'd be happy to add an example / tests here.
An example input would look like:
Closed #306
Changelog description
Adds a new
torsiondrive
compute procedureStatus