Skip to content

Excited State Variable Cleanup for adcc #2513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 8, 2022
Merged

Excited State Variable Cleanup for adcc #2513

merged 7 commits into from
Apr 8, 2022

Conversation

maxscheurer
Copy link
Contributor

@maxscheurer maxscheurer commented Mar 29, 2022 •
edited
Loading

Description

adcc version of #2462, according to standard #2486.

Todos

  • Consolidate excitation energy/property variable names
  • Update tests:
    • port to pytest
    • test all access patterns
    • port PE-ADC tests
  • Update docs

Questions

Checklist

Status

  • Ready for review
  • Ready for merge

loriab
loriab reviewed Mar 29, 2022
View reviewed changes
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.

yes, please, on the "commented out lines" ending in # P::e ADC. Your choice whether you want the ADCC label (we'd have to add that to a list somewhere in the autodocs) or continue with ADC. The main thing on those "commented out lines" is to have a single simple string as the set variable argument. all caps, except for "variables" like root numbers.

JonathonMisiewicz
JonathonMisiewicz reviewed Mar 29, 2022
View reviewed changes
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone Mar 29, 2022
@maxscheurer
Copy link
Contributor Author

Question: Is it worth refactoring adcc ctests to pytest? I will need to touch every adcc test to accommodate for new variables (previously all excitation energies were just stored in a single array, so that we could use compare_arrays) anyways.

@JonathonMisiewicz
Copy link
Contributor

Lori introduced a mechanism to run any ctest through pytest in a recent PR, so I don't think it'd be worth it unless there's a very clear "test template" that all of your tests follow.

@maxscheurer
Copy link
Contributor Author

Yes, I think I'll refactor all the adcc tests using pytest...

@maxscheurer maxscheurer force-pushed the adcc_vars branch 3 times, most recently from fb2197d to f0ba74c Compare March 31, 2022 19:22
@JonathonMisiewicz JonathonMisiewicz dismissed their stale review April 5, 2022 17:17

Stale. I'll want to look over the latest version of this...

@maxscheurer maxscheurer force-pushed the adcc_vars branch 4 times, most recently from 5fb11dd to 89e7243 Compare April 6, 2022 09:31
JonathonMisiewicz
JonathonMisiewicz reviewed Apr 6, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

Thanks for doing all this! The pytests look great.

There's one more thing to be patched, but it's simple.

@maxscheurer maxscheurer requested a review from loriab April 7, 2022 11:53
@maxscheurer maxscheurer added the cleanup For issues where the goal is to make Psi4 a little cleaner. label Apr 7, 2022
loriab
loriab approved these changes Apr 7, 2022
View reviewed changes
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.

nice! thanks for all the standardization and testing.

@hokru
Copy link
Member

hokru commented Apr 8, 2022

Could you please rebase instead of merge with master?

@maxscheurer
Copy link
Contributor Author

Could you please rebase instead of merge with master?

Oh well I'm never going to use the GitHub browser "resolve conflicts" functionality again... that's annoying.

@maxscheurer maxscheurer requested a review from hokru April 8, 2022 09:41
@hokru hokru merged commit dbea82e into psi4:master Apr 8, 2022
@maxscheurer maxscheurer mentioned this pull request Apr 8, 2022
Closed
16 tasks
@maxscheurer maxscheurer deleted the adcc_vars branch April 8, 2022 10:50
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @zachglick
Excited State Variable Cleanup for adcc (psi4#2513)
4c515be 
* excited state var cleanup for adcc

* migrate adcc tests

* fix tests

* docs & tests

* port pe adc tests

* tighter conv tol for reference data/tests

* reviews, complete docs
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @zachglick
Excited State Variable Cleanup for adcc (psi4#2513)
f27ad5b 
* excited state var cleanup for adcc

* migrate adcc tests

* fix tests

* docs & tests

* port pe adc tests

* tighter conv tol for reference data/tests

* reviews, complete docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loriab loriab loriab approved these changes

@hokru hokru hokru approved these changes

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

Assignees
No one assigned
Labels
cleanup For issues where the goal is to make Psi4 a little cleaner.
Projects
None yet
Milestone
Psi4 1.6
Development

Successfully merging this pull request may close these issues.
4 participants
@maxscheurer @JonathonMisiewicz @hokru @loriab