-
Notifications
You must be signed in to change notification settings - Fork 9
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
Bespoke clean up #77
Bespoke clean up #77
Conversation
…e torsions. Correct TD base settings in line with torsiondrive, use qcsubmit optimisation spec.
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.
A few minor suggestions but otherwise LGTM! Thanks for these fixes / improvements!
program: str = Field(..., description="The program to use to evaluate the model.") | ||
model: Model = Field(..., description=str(Model.__doc__)) | ||
|
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.
Did you want to replace these with the QCSpec model at some point?
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.
Yes I think the validation in the QCSpec model should help avoid a lot of errors Ill swap these fields out for a qc_spec
field.
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 is done in df5a6bf but this means that the qc cache now depends on arbitrary fields like the spec name and spec description, maybe the QCSpec object should be split in qcsubmit to offer a version without these fields but which still offers the validation?
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.
Ahh true... yeah I think splitting would be good! Would it be worth revisiting openforcefield/openff-qcsubmit#152 at the same time?
We could also expose it as a setting of the service itself: https://github.com/openforcefield/bespoke-fit/blob/master/openff/bespokefit/executor/services/settings.py |
This pull request introduces 1 alert when merging df5a6bf into dffb29e - view on LGTM.com new alerts:
|
Description
This PR fixes some small issues in the bespokefit pipeline and simplifies some of the models.
Todos
Notable points that this PR has either accomplished or will accomplish.
initial_parameter_values
to return all of the fitting attributes per parameterQuestions
Status