search

theochem / meanfield

HORTON module for SCF and HF/DFT methods
GNU General Public License v3.0
4 stars 1 forks source link

Split out Orbitals #6

Open matt-chan opened 5 years ago

matt-chan commented 5 years ago

It seems the Orbitals class is more popular than we had first anticipated. @FarnazH and @kumrud are both using it in downstream projects (or reproducing functionality from it) by itself.

It might make sense to split out the Orbitals class into its own package. I don't think it's going to be possible to isolate it from meanfield (@tovrstra and I discussed it in Gent), but maybe we can at least make it a separate package, which meanfield will depend upon. That way, people don't need to install libxc to get orbitals related functionality.

Thoughts @tovrstra?

tovrstra commented 5 years ago

This data structure is very light and used extensively in meanfield. Factoring it out and making a general purpose implementation is probably overkill. The common denominator is that you want to store orbital coefficients together with some additional info. E.g. the "additional info" can vary a bit from one application to the other. In meanfield, orbital energies are important, while in other situation they are not. I'm not sure if it worth the trouble to use exactly the same implementation across different projects because we would only be factoring out something fairly small. The benefits are going to be limited compared to the extra complexity.

PaulWAyers commented 5 years ago

So what's the takeaway? I can think of several options: (1) meanfield porcelain makes it easy to get orbitals, and then every "downstream project" has its own orbitals class (or equivalent). (2) orbital class is overkill. Just store MO coefficients, occupation numbers, orbital energies as arrays.

The only thing I think is that it's stupid if people need to use meanfield when all they want is orbitals. If they get orbital information from an external program (IOData) then they shouldn't need meanfield for anything that I can fathom. Probably gbasis is often needed though....though if gbasis is very modular the dependency could be eliminated by duplicated (or copying over) limited functionality for people who need only a tiny bit of the mass of gbasis. [This is also a reason to get gbasis to have less dependencies, so that it is easier to deal with.]

tovrstra commented 5 years ago

IOData doesn't depend on meanfield anymore, so that should be fine.

PaulWAyers commented 5 years ago

My understanding was that ChemTools (and probably other people) were using features in IOData that then Matt took out and are now only in MeanField or ... I'm not sure, maybe they got put on the "meanfield" side instead of the "IOData" side in the module-building. My impression was that there were things in MeanField that ChemTools (and similar) needed...which isn't at all ideal...

Maybe @matt-chan or @FarnazH or @kumrud can be more specific on what is going on.

tovrstra commented 5 years ago

Good, I'd love to hear some specific use cases and continue from there. Orbitals (or expansions as they were called then) were taken out of IOData to avoid that Meanfield and IOData would become each other's dependencies, such that one cannot use one without the other. That would practically mean they become one module.

matt-chan commented 5 years ago

The only use I'm aware of other than the energies/coeffs/occs was the exp_alpha/exp_beta.to_dm(). @FarnazH can be more specific though. I'm also not sure if we are also moving towards transferring 2DMs.

FarnazH commented 5 years ago

I agree that having an Orbital package is an overkill because orbital information is just a couple of arrays (energies, coefficients & occupations). It might be very useful to have a (Molecular) Orbital class though implemented in iodata package. There are things like 1DM, 2DM, HOMO/LUMO energy/index of alpha/beta electrons, etc. that can be easily implemented there... This will simplify iodata wrappers in ChemTools, and in addition, adds useful functionality for independent use of iodata package.