Skip to content

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