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

Added to_cisdtq functionality to RFCI wave function types for extract… #69

Merged
merged 77 commits into from
Apr 15, 2023

Conversation

ghb24
Copy link
Contributor

@ghb24 ghb24 commented Nov 3, 2022

…ion of C1 -> C4 info

vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
George Booth added 4 commits November 17, 2022 13:53
…All seems to be working correctly. Tests and examples to come.
…er than ndarray. Rather than get from HDF5 file, just use try/except block for the time being, and fix properly later...(presume whether it stores as ndarray or hdf5 will depend on what it thinks is available memory).
type of correction == 'external-ccsdv', where the parent Coulomb integral is contracted.
If not passed in, MO energy if needed will be constructed from the diagonal of
get_fock() of embedding base class, and the eris will be also be obtained from the
embedding base class. Optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not always just use the embedding class attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my question was why bother with eris.mo_energy at all, and not always just get the MO energies from the embedding class - actually, you can get them also from the fragment, with the function get_fragment_mo_energy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the eri's themselves are potentially used here too, when we are contracting with the low-level coulomb integrals. I think for solids, there may be differences in MO energies regarding whether they are the shifted ones or not?

vayesta/solver/coupling.py Outdated Show resolved Hide resolved
vayesta/solver/coupling.py Outdated Show resolved Hide resolved
@maxnus
Copy link
Contributor

maxnus commented Apr 3, 2023

@obackhouse I added a few comments, but looks good (and like a lot of work!) well done

@cjcscott
Copy link
Contributor

Hey all, it seems like a general consensus has been reached that all functionality in here is stable and ready to go, so unless there are any complaints (or anyone would like to do it before me) I'm happy to merge it into #65 branch tomorrow. I've resolved any conflicts in this merge already (these were just the changes to the CI triggering for PRs not into master or dev, and both branches implementing the same fix for the pyscf symmetry operation renaming). We can then move on to merging that branch into master, which I'm hoping to be relatively straightforward- though we should probably add these changes into the changelog for that version before merger if we want to include them in that release. Would @abhishekkhedkar09 or @obackhouse be happy to write that?

@obackhouse
Copy link
Contributor

I'm still waiting on @maxnus to respond to a couple of comments above (though they're minor), and there are unresolved comments about the naming convention and the integral transformation that I think @ghb24 was going to look at. After that, LGTM

@obackhouse
Copy link
Contributor

oh and the changelog can just go in the release tag?

@cjcscott
Copy link
Contributor

I've also just noticed my conflict resolution commit has triggered possibly the first CI test run on this branch, since they didn't run after your fix for some reason, so we might have some fixes to make there as well!

@cjcscott
Copy link
Contributor

oh and the changelog can just go in the release tag?

As in we don't need to do it until actually the commit where we release v1.0.2? Yeah, I just meant we shouldn't forget to do that if we're sticking to version-specific changelogs. I'm assuming release would correspond to merging #65 into master?

@obackhouse
Copy link
Contributor

oh and the changelog can just go in the release tag?

As in we don't need to do it until actually the commit where we release v1.0.2? Yeah, I just meant we shouldn't forget to do that if we're sticking to version-specific changelogs. I'm assuming release would correspond to merging #65 into master?

Even further, usually you merge into features straight into master (rather than via a v1.0.2 branch) and then when you want a new release, tag a release with master, and write a changelog since the last one

@cjcscott
Copy link
Contributor

Yeah, I don't disagree- I'd say lets stick with merging straight to master in future.

I've done some tracing down on the test failures here, but am going to open a new PR to avoid too much discussion here since it mainly concerns the spin-symmetry-broken PR that was merged in here a while back.

@maxnus
Copy link
Contributor

maxnus commented Apr 12, 2023

I'm still waiting on @maxnus to respond to a couple of comments above (though they're minor), and there are unresolved comments about the naming convention and the integral transformation that I think @ghb24 was going to look at. After that, LGTM

I'm happy for this to be merged!

…ured TestMolecule.uhf_stable uses symmetry-broken reference.
@cjcscott cjcscott mentioned this pull request Apr 14, 2023
George Booth added 3 commits April 14, 2023 15:59
Now, there is just an 'external' keyword for the external correction.

Instead, whether the interaction in the T3 * V term is contracted
with V in the high-level (i.e. FCI) 'smaller' space, or the low-level
(i.e. CCSD) 'larger' space, is controlled via an optional argument to the
'externally_correct' function. This argument is 'low_level_coul', and
defaults to 'True'. This means that the default behaviour is equivalent
to the 'external-ccsdv' correction previously. Tests and examples updated.
with translationally equivalent fragments.
@ghb24
Copy link
Contributor Author

ghb24 commented Apr 14, 2023

Lets merge this, once we are sure tests pass.

George Booth and others added 2 commits April 15, 2023 11:30
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2023

Codecov Report

Patch coverage: 91.21% and project coverage change: +2.27 🎉

Comparison is base (3d878c2) 69.29% compared to head (0c962df) 71.56%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff             @@
##           v1.0.2a      #69      +/-   ##
===========================================
+ Coverage    69.29%   71.56%   +2.27%     
===========================================
  Files          131      133       +2     
  Lines        17395    18439    +1044     
  Branches      2513     2570      +57     
===========================================
+ Hits         12053    13195    +1142     
+ Misses        4612     4506     -106     
- Partials       730      738       +8     
Impacted Files Coverage Δ
vayesta/core/qemb/qemb.py 73.34% <0.00%> (-0.09%) ⬇️
vayesta/misc/molecules/molecules.py 57.31% <10.00%> (-3.08%) ⬇️
vayesta/core/scmf/scmf.py 93.82% <40.00%> (-3.58%) ⬇️
vayesta/core/qemb/fragment.py 75.32% <50.00%> (-0.28%) ⬇️
vayesta/solver/ccsdtq.py 75.66% <75.66%> (ø)
vayesta/ewf/fragment.py 67.86% <88.88%> (+1.09%) ⬆️
vayesta/core/util.py 75.82% <92.53%> (+3.15%) ⬆️
vayesta/core/types/wf/fci.py 79.48% <93.13%> (+30.47%) ⬆️
vayesta/solver/coupling.py 82.00% <95.97%> (+42.51%) ⬆️
vayesta/core/types/wf/ccsdtq.py 83.87% <100.00%> (+38.87%) ⬆️
... and 4 more

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ghb24 ghb24 merged commit edb1627 into v1.0.2a Apr 15, 2023
@obackhouse obackhouse deleted the to_cisdtq branch April 16, 2023 15:17
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.

6 participants