-
Notifications
You must be signed in to change notification settings - Fork 70
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
ResultProperties: Added keywords for CCSD quantities. #54
Conversation
I think this should a PR to QCSchema first. A few of those quantities will need string explanations with them and there will be some feedback there I am guessing. |
I have made a PR to QCSchema for the CCSD quantities (MolSSI/QCSchema#61). |
This should be good to update. |
So I have added the CC keywords to QCElemental. Should I also go ahead with the renaming of |
yes, please. any and all changes to make qcel agree with the qcschema PR. |
…gy to follow QCSchema
Okay I've updated the mp2 to match the QCSchema PR. Presumably this will cause some problems in other parts of the QCArchive. |
qcelemental/models/results.py
Outdated
ccsdt_correlation_energy: float = None | ||
ccsdt_total_energy: float = None | ||
ccsdt_dipole_moment: List[float] = None | ||
ccsdt_iterations: int = None |
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.
CCSDT(Q) last one (ugh).
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.
Is this for completions sake? Since it looks like there a number of other CC methods out there too.
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.
Yea, honestly scratching my head on this one a bit after seeing it in practice. Anything beyond CCSD(T) might be better served in qcvars
. @loriab?
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.
just checked and CCSDT(Q) wasn't added to qcschema. I think it's ok to skip for now.
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.
Yeah it doesn't seem there are a lot of codes out there that support past CCSD(T). I guess if we are still on this topic I'd only advocate for the addition of CC-F12 methods.
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.
in my experience, these are what you want to collect for f12 methods from molpro. not sure what balance of work-with-this-and-learn-from-practice or add-em-now to suggest.
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.
oh, and one shouldn't actually be storing (T), (T*), (T**), as those can be built by equation
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.
lgtm. I think CCF12 and/or (Q) can go in another round. Current can be a start for the next qcel release. If you make any other changes, please go ahead and add a line for the next release https://github.com/MolSSI/QCElemental/blob/master/docs/source/changelog.rst .
I'm happy to leave as is and add in more in later rounds. So to clarify do I need to add a line for the current PR to https://github.com/MolSSI/QCElemental/blob/master/docs/source/changelog.rst ? |
Lets leave it as is, but snip off everything after CCSD(T). The changeling line would be great! |
Okay so snip out stuff after CCSD(T) here in |
Added keywords for CCSD quantities. Is this all that needs to be done? Or does the qc_schema also need to be updated?