search

BoothGroup / Vayesta

A Python package for wave function-based quantum embedding
Apache License 2.0
33 stars 8 forks source link

Deprecation and cleanup #95

Closed obackhouse closed 1 year ago

obackhouse commented 1 year ago

I'd like to open this as a discussion and a collaborative effort to clean up code that is no longer in use or has been deprecated for a long time.

Examples which are still not running: edmet

ewf/molecules

ewf/other

obackhouse commented 1 year ago

@maxnus what is the status of the WIP, OBSELETE and TODO.txt in vayesta/ewf? Can these be removed, or moved to some development branch?

obackhouse commented 1 year ago

@maxnus same question as above, but for vayesta/core/DEL?

cjcscott commented 1 year ago

We should also add in commented old code into this- off the top of my head the obsolete kernel and get_init_guess in ewf/fragment.py, and the PMO (I'm assuming projected molecular orbital?) fragmentation functionality in ewf/ewf.py.

Given none of these examples are currently in use (or should be renamed if not) I'd be very pro just removing them. In all cases they're available in v1.0.0 or v1.0.1 if we want to check what they contain.

maxnus commented 1 year ago

This is a good idea! I made a PR.

I kept some of these files and commented code, since you quickly start to forget previous work, once it's just part of the git history and not that visible anymore. Really these should probably be stale branches.

PMO indeed stands for projected MO, a la regional embedding from Tim Berkelbach. I compared their performance to MP2 BNOs in the past, but the BNOs were much better in the tested systems.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -72.13 :warning:

Comparison is base (ed2f91f) 72.12% compared to head (782c27e) 0.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #95 +/- ## ========================================== - Coverage 72.12% 0.00% -72.13% ========================================== Files 138 138 Lines 18476 18337 -139 Branches 2578 0 -2578 ========================================== - Hits 13325 0 -13325 - Misses 4420 18337 +13917 + Partials 731 0 -731 ``` | [Impacted Files](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup) | Coverage Δ | | |---|---|---| | [vayesta/\_\_init\_\_.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9fX2luaXRfXy5weQ==) | `0.00% <0.00%> (-76.54%)` | :arrow_down: | | [vayesta/core/ao2mo/\_\_init\_\_.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2FvMm1vL19faW5pdF9fLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [vayesta/core/ao2mo/helper.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2FvMm1vL2hlbHBlci5weQ==) | `0.00% <0.00%> (-75.77%)` | :arrow_down: | | [vayesta/core/ao2mo/kao2gmo.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2FvMm1vL2thbzJnbW8ucHk=) | `0.00% <0.00%> (-90.73%)` | :arrow_down: | | [vayesta/core/ao2mo/postscf\_ao2mo.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2FvMm1vL3Bvc3RzY2ZfYW8ybW8ucHk=) | `0.00% <0.00%> (-74.38%)` | :arrow_down: | | [vayesta/core/bath/\_\_init\_\_.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2JhdGgvX19pbml0X18ucHk=) | `0.00% <0.00%> (-72.73%)` | :arrow_down: | | [vayesta/core/bath/bno.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2JhdGgvYm5vLnB5) | `0.00% <0.00%> (-85.27%)` | :arrow_down: | | [vayesta/core/bath/dmet.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2JhdGgvZG1ldC5weQ==) | `0.00% <0.00%> (-63.13%)` | :arrow_down: | | [vayesta/core/bath/ewdmet.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2JhdGgvZXdkbWV0LnB5) | `0.00% <0.00%> (-26.54%)` | :arrow_down: | | [vayesta/core/bath/full.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL2JhdGgvZnVsbC5weQ==) | `0.00% <0.00%> (-95.24%)` | :arrow_down: | | ... and [88 more](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup) | | ... and [37 files with indirect coverage changes](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/95/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup)

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

cjcscott commented 1 year ago

Just as a follow-on from the SCMF stuff, the functionality https://github.com/BoothGroup/Vayesta/blob/be227313c17c10d183e3dc9c7a522099cc426b6a/vayesta/core/qemb/qemb.py#L228-L235 in Embedding.__init__ doesn't seem to be used anywhere, let alone tested, and as demonstrated in #91 uses dodgy syntax. It seems ripe for depreciation unless we have any good reasons not to.

obackhouse commented 1 year ago

I've removed core/DEL and core/ao2mo/DEL - let me know if you want me to restore these into a stale branch. Otherwise at least this PR serves as a checkpoint for removed functionality so they won't be completely lost to git history...

obackhouse commented 1 year ago

What's the deal with add_tsymmetric_fragments? It has a deprecation warning but as far as I can tell it's the intended function, were make_tsymmetric_fragments is the deprecated one?

cjcscott commented 1 year ago

I think the preferred interface is currently to use emb.symmetry.set_translations(supercell_mesh) prior to initialising any fragmentations, which will then automatically include all translationally related fragments. This uses qemb.create_transsym_fragments rather than add_tsymmetric_fragments or make_tsymmetric_fragments, so I think we should depreciate both of those functions entirely.

I think I have an OK idea of how the different symmetries in Vayesta are being applied- translational symmetry is the only one using this approach (termed being applied in fragment context here, since it should apply to all included fragments) while rotation, inversion and mirror symmetries are handled via context managers within the fragmentation itself (termed as being within their own context).

However, as I don't think think there was much review done on these changes the examples and tests don't all reflect these updates- @basilib has caught some of them in #105 (eg. no longer supported approaches to rotational symmetries) but we should make a concerted effort to clean all this up here, as it is very unclear due to the number of old interfaces. If we then fix any tests broken by this, then merge this and fix any broken examples along with the rest in the above PR that seems sensible to me? I've also mentally added updating the docs to have a proper discussion of this to my TODO list...

cjcscott commented 1 year ago

I've updated this branch to remove conflicts with master; I'd suggest we merge this once tests pass, then I'll open a PR for clarifying the symmetries functions, as that's a suitably different set of things and can also involve updating the docs.

basilib commented 1 year ago

I merged the example fixes into this branch, but there are still a few which require attention. I also wrote a quick script to run the examples.

basilib commented 1 year ago

The interface for rotational symmetry has been changed to no longer accept letters (x,y,z) to specify the rotation axis and instead requires the use of an explicit vector. I have changed the examples to use explicit vectors but the tests use letters.

Is this an intended change or do we want to keep the option of specifying the axis by a letter?

maxnus commented 1 year ago

I think the preferred interface is currently to use emb.symmetry.set_translations(supercell_mesh) prior to initialising any fragmentations, which will then automatically include all translationally related fragments. This uses qemb.create_transsym_fragments rather than add_tsymmetric_fragments or make_tsymmetric_fragments, so I think we should depreciate both of those functions entirely.

I think I have an OK idea of how the different symmetries in Vayesta are being applied- translational symmetry is the only one using this approach (termed being applied in fragment context here, since it should apply to all included fragments) while rotation, inversion and mirror symmetries are handled via context managers within the fragmentation itself (termed as being within their own context).

Yes, this is correct 👍 All symmetry-related fragments are added via qemb.create_symmetric_fragments at the moment, which is invoked via creating fragments within a symmetry context manager

ghb24 commented 1 year ago

This is good to know, but also aware that once this PR is merged the discussions on this thread re. implementation of translational/other symmetry will be essentially lost! We need to make sure that this is clearly described in the docstrings / documentation / examples going forward...

cjcscott commented 1 year ago

As I said above, I think at the same time as depreciating the alternative symmetry interfaces we should add a section into the quick start documentation discussing symmetry considerations. However, I'm a little wary that this PR already makes a lot of progress and delaying it for this purpose isn't actually beneficial, so would suggest merging this (once all examples are fixed) then I'll open a new PR updating symmetry stuff.

cjcscott commented 1 year ago

I've updated the EDMET examples. Otherwise I've found:

I would suggest that once the final examples are resolved we merge this branch; I'll then get a PR for removal of old symmetry routines up and crack on with that.

abhishekkhedkar09 commented 1 year ago

I've updated the EDMET examples. Otherwise I've found: -the tailoring examples appear to have had the same convergence problems on the commit they were introduced in (1e0c76a, see #68 as well...), so I think scrap them and start again. @abhishekkhedkar09 do you have any good scripts as an example of tailoring? I know you don't use delta tailoring so much, so maybe not for that. -the population analysis functionality appears have been broken since the removal of ActiveSpace objects in favour of Cluster objects in 796e9a8. I would suggest fixing this probably falls outside the remit of this PR, but is definitely worthwhile; I don't think it'll be much effort(just requires adding functionality to add in the environmental 1rdm contribution) but is worth doing robustly. I'd suggest we leave this for now; I'll raise an issue to make sure it's documented, this might be a nice short PR for someone who wanted to have a go at it. -The ewf/other examples have a comment saying they were broken from from 3d82332 (4th December 2021) so I've deleted them; I don't think it's worth the effort to fix them. I've also added rotational symmetry to 71-sc-h-ring.py for computational efficiency.

alright.. I will add an illustrative example for Tailored CC.

obackhouse commented 1 year ago

I think I am done - what's the status on the examples?

cjcscott commented 1 year ago

I've just tweaked the delta tailoring example, and opened issues (#110 and #111) for the topics we've discussed here but not yet fixed. As such, I think this is ready to be merged.

obackhouse commented 1 year ago

Ready to merge then