BoothGroup / Vayesta

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

Change fragment definitions #14

Closed maxnus closed 2 years ago

maxnus commented 2 years ago

As we move to making this repo public, there is one (hopefully last) change I would like to make to the fragment definitions. Syntactically the suggested change looks like this:

Before:

emb = EWF(mf)
emb.iao_fragmentation()
emb.add_atomic_fragment(0)
emb.add_orbital_fragment('Fe 3d')
emb.kernel()

After:

emb = EWF(mf)
with emb.iao_fragmentation() as f:
    f.add_atomic_fragment(0)
    f.add_orbital_fragment('Fe 3d')
emb.kernel()

It doesn't add any lines, but makes the relationship between the fragmentation and fragment definitions more clear. This may be especially important for mixed fragmentations, such as:

emb = EWF(mf)
with emb.iao_fragmentation() as f:
    f.add_atomic_fragment(0)
with emb.iaopao_fragmentation() as f:
    f.add_atomic_fragment(1)
emb.kernel()

instead of:

emb = EWF(mf)
emb.iao_fragmentation()
emb.add_atomic_fragment(0)
emb.iaopao_fragmentation()
emb.add_atomic_fragment(1)
emb.kernel()

Apart from the changed syntax, this change would allow removing the fragmentation state (stored in emb.fragmentation) from the embedding - this is generally a good thing, less state, less side-effects, less errors and more understandable code. After all, the fragmentation state is not used anywhere, except when defining new fragments.

It would also move the fragment defining functions, add_atomic_fragment, add_orbital_fragment, etc, from the embedding class to the fragmentation class. This is nice, because a) they don't really fit into the embedding class that well, and b) they can be fragmentation specific. For Charlies' new cas_fragmentation, add_atomic_fragment doesn't make any sense, and vice versa for add_cas_fragment and iao_fragmentation.

cjcscott commented 2 years ago

This sounds good to me- as you say, we never use the fragmentation elsewhere, and avoids lots of edge cases if we have functionality that's only compatible with some fragmentations. Given this makes it much more natural to use mixed fragmentations adding a check that any added fragment is orthogonal to any existing ones might might make the limitations of this more obvious to anyone using it.

ghb24 commented 2 years ago

Sounds good to me...!

maxnus commented 2 years ago

This is now changed in 5d798f2