Skip to content
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

Update TDKeywords #283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update TDKeywords #283

wants to merge 1 commit into from

Conversation

jthorton
Copy link
Contributor

@jthorton jthorton commented Nov 26, 2021

Description

This PR just updates the TDKeywords model to be consistent with the qcfractal model as converting between the two causes issues currently.

Changelog description

TDKeywords now accepts additional keywords in line with qcfractal.

Status

  • Code base linted
  • Ready to go

@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #283 (0ca8035) into master (cabec4a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@bennybp
Copy link
Contributor

bennybp commented Dec 13, 2021

I've been thinking about this. I'm not sure the additional keywords actually belongs in the TDKeywords. The additional keywords should just be folded into the optimization keywords, right?

I know that was a change we made to QCFractal. Thinking about it, I'm not positive that part was necessary, and need to revisit it for the new version (with input from @dotsdl )

@jthorton
Copy link
Contributor Author

jthorton commented Dec 16, 2021

The additional keywords should just be folded into the optimization keywords, right?

Yeah, that's how it should be done and thats basically what TDKeywords does with the dihedral field. This is just turned into a constraint and passed to the optimisation keywords. I think in QCFractal the additional keywords should be separate from the optimisation for the same reason the dihedral field is. As if this is unique for every entry in a collection of size N this would require N different optimisation specs which only differ by the additional keywords. So it makes more sense to have one consistent optimisation specification and have the additional keywords separate.

In the case of single jobs via QCengine then its fine to combine into the optimisation specification, however I was thinking it might be best to unify all of the models, then users can create a single specification that can be run locally via qcengine or via qcfractal.

@loriab loriab added the wontfix This will not be worked on label Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants