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

Excited State Final Cleanup #2569

Merged
merged 11 commits into from
May 19, 2022
Merged

Excited State Final Cleanup #2569

merged 11 commits into from
May 19, 2022

Conversation

JonathonMisiewicz
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz commented May 5, 2022

Description

This will eventually conclude the standardizing of excited state variable names across all of Psi and the docs.

Right now, this just standardizes TD-DFT psivars for final comments. This is a short-and-sweet spec of the new standard that I'm putting up for comments. (Largely from @loriab)

Todos

  • Excited state psivar final cleanup

Checklist

  • TDDFT tests pass
  • ADC tests pass
  • EOM tests pass

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone May 5, 2022
@JonathonMisiewicz
Copy link
Contributor Author

Still in the draft stage, but next round of edits is in. I need to give the tests another pass to make sure we have proper coverage.

The other major issue is the docs. I'll need to update the section describing excite state psivars, but @loriab, how do you feel about grouping psivars together? See discussion here.

@loriab
Copy link
Member

loriab commented May 18, 2022

The other major issue is the docs. I'll need to update the section describing excite state psivars, but @loriab, how do you feel about grouping psivars together? See discussion #2462 (comment).

I'd still strongly favor something like the below. It lets the variables (n,m,h,i) be defined in the definition, and new methods can join existing psivar entries. Also just less visual clutter when all but the methods line up. What do you think?

.. psivar:: ADC ROOT n TOTAL ENERGY
    TDDFT ROOT n TOTAL ENERGY
    TD-fctl ROOT n TOTAL ENERGY
    CCSD ROOT n TOTAL ENERGY
    ...

    def incl n

.. psivar:: TDDFT ROOT n (h) -> ROOT m (i) OSCILLATOR ENERGY
    CCSD ROOT n (h) -> ROOT m (i) OSCILLATOR ENERGY

    def incl n, m, h, i

@JonathonMisiewicz
Copy link
Contributor Author

I wish there was a way to cut down the repetition in variable description, but from the user standpoint, that probably is the way to go...

I'll get docs up and un-draft this (hopefully tonight) and save test pass for tomorrow.

@loriab
Copy link
Member

loriab commented May 18, 2022

I wish there was a way to cut down the repetition in variable description, but from the user standpoint, that probably is the way to go...

In another project, I have the glossary-like doc generated by python to cut down on that. Perhaps someday ...

@JonathonMisiewicz
Copy link
Contributor Author

Docs took way longer than expected, but that part is done. Now to give tests a sweep.

@JonathonMisiewicz JonathonMisiewicz marked this pull request as ready for review May 18, 2022 21:33
Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

congratulations and thanks on wrangling a rather mind-numbing PR. I know you have another pass planned, so here's some first-look comments.

.. psivar:: ADC ROOT 0 (h) -> ROOT m (i) OSCILLATOR STRENGTH (VEL)
TD-fctl ROOT 0 (h) -> ROOT m (i) OSCILLATOR STRENGTH (VEL)

The velocity-gauge oscillator strength of the transition from root *n* to root *m*,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definition mentions n but variables fixed at n=0. generalize variables?


.. psivar:: ADC ROOT 0 -> ROOT m ROTATORY STRENGTH (VEL)
CCname ROOT n -> ROOT m ROTATORY STRENGTH (VEL)
TD-fctl ROOT 0 -> ROOT m ROTATORY STRENGTH (VEL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing def


Redundant quadrupole array [e a0^2] for the requested coupled cluster level of theory and root *n*, and the transition is of irrep *h*, (*n* starts at 0), (3,3).

.. psivar:: CCname ROOT m -> ROOT n EINSTEIN A (LEN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these m -> n for a reason (instead of n -> m as above)? if anything, spectroscopic notation is to <- from, but I think we're steady on from -> to

tests/cc56/input.dat Outdated Show resolved Hide resolved
doc/sphinxman/source/glossary_psivariables.rst Outdated Show resolved Hide resolved
Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excited state QCVars much improved compared to the start of this saga. And I think the (IN h) is a nice clear marker of the intra-irrep numbering scheme.

tests/cc56/input.dat Outdated Show resolved Hide resolved
Copy link
Contributor

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this! LGTM!

@JonathonMisiewicz JonathonMisiewicz linked an issue May 19, 2022 that may be closed by this pull request
16 tasks
Copy link
Member

@jturney jturney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@loriab loriab merged commit f251cf4 into psi4:master May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excited State Variable Cleanup
4 participants