Stresses API

[antoine-levitt]

Built on top of #486

@niklasschmitz I'm getting an error in one of your fallbacks (looks like it's because I forward differentiate wrt several parameters at once), can you take a look?

• Add tests
• Add unfolding

[niklasschmitz]

Indeed my norm rule hadn't covered the case of multiple partials in a Dual yet, I'll extend it shortly!

[antoine-levitt]

Stresses are not correct with symmetries, as expected:

julia> ans
3×3 Matrix{Float64}:
 -0.00159939   7.98518e-5   7.98074e-5
  7.98518e-5  -0.00159939   7.98366e-5
  7.98074e-5   7.98366e-5  -0.00159939
julia> ans
3×3 Matrix{Float64}:
 -0.00181531   -0.000146066  -0.000119765
 -0.000146066  -0.00151338    0.000505329
 -0.000119765   0.000505329  -0.00146946

without and with symmetries, respectively, for a compressed silicon (a=9.26). The first is correct: negative stresses (and therefore positive pressure) that are only in the diagonal direction (because only a is a free parameter)

[antoine-levitt]

So this needs an unfold_BZ(scfres), which is also needed for @LaurentVidal95 's Wannier PR anyway

[mfherbst]

I guess one should also have a stresses_cart for consistency?

[antoine-levitt]

I'm not super confortable with stress definitions so I'd rather have just one that matches what's found in the literature. Forces are more intuitive

[mfherbst]

I see, but forces in the literature are usually cartesian as well. Perhaps we should only support cartesian forces? Or we wait until we understand more and decide then.

[antoine-levitt]

Well OK, cartesian stresses might make sense. Forces in reduced coordinates definitely makes sense, it's what you want to do for geometry optimization for instance. For stresses I'm not sure what to do about the division by volume, so let's leave it as is for now.

[antoine-levitt]

TODO (reminder to self) add tests

[antoine-levitt]

Good to go from my end

[mfherbst]

I'm not yet fully convinced about the name unfold_BZ, because nothing is really done to the BZ. It is only the symmetry that is undone, so IR-wedge -> full BZ. Maybe remove_symmetries_scfres ? disable_symmetries_scfres ?

[mfherbst]

How about better calling this flag apply_symops or something like that.

[antoine-levitt]

I don't want to give the impression this is a general thing : it's really specialized for psi

[mfherbst]

I think I would structure this differently. For me these three objects are inherently connected and I think it makes sense to fuse the loop, i.e. to handle them simultaneously. That way you also get rid of this strange is_\psi thingy. Also what other k-Point specific objects will we need to transfer in such an unfolding? I don't thing any more.

[antoine-levitt]

So you'd just merge the unfold_BZ and unfold_array functions? I kind of like the separation. You could have other things, like a partial density, a matrix element for a response computation, etc

[mfherbst]

True, but in this setup you do a lot of duplicate work unfold_mapping is computed each unfold_array call. I can see us quickly overusing unfold_BZ, so I'm not too happy about this structure. If you have the unfolding as the outer loop it's still easy to add an extra line if you have an additional entry in your scfres.

[antoine-levitt]

Unfold_mapping is cheap. I definitely would wait until we have a better idea of the use cases for this before doing any refactoring. Not convinced merging functions would help here

[mfherbst]

OTOH is_psi flag is really annoying and the wrapping it into two function names as well. It feels very hackish and I'm not a fan of this solution. If you swap inner and outer loop than you have that info and do not need that strange nested function call structure. I think that is the easiest solution and as you say, when we need more we refactor.

[antoine-levitt]

I've removed the extra functions which were more confusing than anything (trying to pretend this is a high-level API even if it's pretty hacky). But I really don't want to merge the functions : this is already a pain to write and read, having an extra function does help I feel

[antoine-levitt]

I'm not yet fully convinced about the name unfold_BZ, because nothing is really done to the BZ. It is only the symmetry that is undone, so IR-wedge -> full BZ. Maybe remove_symmetries_scfres ? disable_symmetries_scfres ?

Well the irreducible BZ gets unfolded into the reducible BZ, so the name works, no? Nothing is done to the symmetries field of scfres, so disable_symmetries would be a bit misleading

[antoine-levitt]

Review done, thanks!

[mfherbst]

Well the irreducible BZ gets unfolded into the reducible BZ, so the name works, no? Nothing is done to the symmetries field of scfres, so disable_symmetries would be a bit misleading

ok yes, it's only in the scfres where things are really happening that is true. So then unfold_bz_scfres (to match with our capitalisation convention in function calls).

[antoine-levitt]

Not a fan of unfold_bz_scfres(scfres). The long-term plan is for scfres to get a struct, so that this becomes a dispatch.

[mfherbst]

ok then leave off the scfres if you want.