JuliaMolSim / DFTK.jl

Density-functional toolkit
https://docs.dftk.org
MIT License
426 stars 89 forks source link

Structure for term-wise quantities #784

Open mfherbst opened 1 year ago

mfherbst commented 1 year ago

In #766 it came up that it might be useful to have a structure for term-wise quantities similar to what we have for the energies.

Let's start by what we want. From my point of view:

In fact I never was a huge fan on how we handle the Energies right now. It seems like a lot of boiler plate for not giving much functionality. I see the following options:

@azadoks @antoine-levitt I still lean to the second, but let's collect ideas.

GVigne commented 1 year ago

It's going to be hard to have a unified GPU/CPU code with this sort of thing. Dictionaries behave quite badly on the GPU, as they break kernels by adding local effects. Plus, we probably will have memory problems as those dictionaries would have to be stored on the GPU. It's an issue that we are going to have in #766 with the form factors. They way I see it:

mfherbst commented 1 year ago

Ok splitting the code base for something as simple such as storing such contributions sounds like an overkill. The advantage of a structure for term-wise quantities is that we can avoid using dicts by actually having two vectors (keys and values).

Let's keep the form factor discussion separate from this one as it's not user facing (and thus justifies other solutions).

antoine-levitt commented 1 year ago

How about not having the decomposition at all? Ie energy_hamiltonian returns the aggregate (the sum). Then we add energy_per_term(scfres) that recomputes the energies (should be pretty cheap) and does the pretty printing. Then, if we really feel the need, we add a stresses_per_term, or an option to stresses that gives the terms or something (I personally don't see the need, it's easy enough to hack in). There's not a lot of code to be factored there, so simplest is best...

mfherbst commented 1 year ago

Hmm. Ok for energy_hamiltonian, but for user facing functions (scf result, forces, stresses) we should have an intuitive way to get these terms in my opinion. Calling an extra separate function for this does not sound like a convenient solution to me given how often users from the application side are interested in this type of stuff.

What's the issue with just having a struct containing two vectors and a few support functions. We can probably settle down on something even simpler than Energies.

antoine-levitt commented 1 year ago

Feels like overengineering we're going to regret. What's wrong with having an extra function? I kind of like the way we have it now - we do an SCF, that computes an scfres that has the bare minimum, then whatever else you want to do you call an extra function.

mfherbst commented 1 year ago

Yes that I like, too. I fear though this leads to a combinatorial zoo of methods (energy terms, forces total, forces terms, stresses total, stresses terms, ...), which makes it in particular tricky for novice users to find what they want (unlike just navigating through what is stored in the scfres). In particular application users expect to quickly get an overview of the solution they found.

But of course one way out could be to have an endorsed "summary" function people should call on the scfres (which would become the default print function once the scfres is a struct on its own).