brille / brille

A library for symmetry operations and linear interpolation within an irreducible part of the first Brillouin zone.
https://brille.github.io/
GNU Affero General Public License v3.0
16 stars 3 forks source link

Add option to output eigenvectors in Cartesian #86

Closed rebeccafair closed 1 year ago

rebeccafair commented 2 years ago

Many codes that might want to benefit from Brille require eigenvector input/output in Cartesian coordinates rather than the unit cell, so have to convert between them. This conversion is surprisingly expensive as eigenvectors are so larger, so it might be more efficient for Brille to do this conversion internally in parallel.

See https://github.com/pace-neutrons/Pace-Project-Plan/issues/192#issuecomment-1307384194 for more detailed reasoning.

g5t commented 1 year ago

The symmetry operations of a system operate on different values in different ways, depending on the type of the value. Real space vectors are transformed differently than reciprocal space vectors; as are real and reciprocal pseudo-vectors; or the eigenvectors of the grand dynamical matrix. In an attempt to distinguish between all possible cases, brille uses an enum class with values RotatesLike::Real, RotatesLike::Reciprocal, RotatesLike::Axial, and RotatesLike::Gamma to represent real vectors, reciprocal vectors, reciprocal pseudo-vectors, and (real) eigenvectors of the grand dynamical matrix, respectively.

I did not originally consider the possiblity that any of the above quantities would be expressed in units other than fractions of their basis vectors, that is real lattice units or reciprocal lattice units; but now see that this option would be very useful. The question then becomes how-best to support values expressed in a Cartesian coordinate system. I see three possible solutions

  1. Require Cartesian coordinate input
  2. Extend the RotatesLike enumeration to include RotatesLike::CartesianGamma, etc.
  3. Split the enumeration into two orthogonal enumerations, e.g, enum class brille::ValueType {vector, pseudovector, Gamma} and enum class brille::LengthUnit {none, angstrom, inverse_angstrom, real_lattice, reciprocal_lattice}

If the first option is chosen, future use cases may suffer from the same issue that #88 solves. The second option may lead to a very complex RotatesLike enumeration with hard-to-rationalise item names. This leads me to prefer the third option (which extends the existing enum class LengthUnit); but this solution is likely to add API complexity, which may be unacceptable.

What do we think @rebeccafair @mducle ? Is the improved clarity and flexiblity of using two enumerated values worth the extra parameter when configuring brille to interpolate data?

rebeccafair commented 1 year ago

I agree 1. is out.

  1. to me sounds like the best option for now, as it is a sensible extension of the current interface, requires the least amount of code and in the interests of time will allow us to get a Brille release out quickly, and finish/test/profile https://github.com/pace-neutrons/Euphonic/pull/104 before I leave. The RotatesLike enumeration is currently manageable and we don't have any specific other use cases for it right now, so I think this is the least painful option.

  2. Is definitely an improvement in the interface, but also a big change, if we can't think of any other pressing use cases right now it's probably too much of a complex change to implement in the time we have. Maybe make an issue for the next major release of Brille?

mducle commented 1 year ago

I think I would prefer option 3, or perhaps a modification where you've got the current RotatesLike:: enums and an additional two enums, RotatesLikeUnits::LatticeUnits and RotatesLikeUnits::Cartesian since in we only want to distinguish 8 different categories at the moment rather than the 15 which option 3 can handle - e.g. I think we don't support Gamma-angstrom, Gamma-real_lattice, or pseudovector-inverse_angstrom, pseudovector-reciprocal_lattice, and I don't know what the none units would correspond to.

However, as Becky said, I think that in the interest of time, that option 2 is better for now.

g5t commented 1 year ago

The current relationship between RotatesLike enumerated values and the two proposed enumerations is:

LengthUnit:: ValueType::vector ValueType::pseudovector ValueType::Gamma
::none*
::angstrom*
::inverse_angstrom*
::real_lattice RotatesLike::Real RotatesLike::Axial RotatesLike::Gamma
::reciprocal_lattice RotatesLike::Reciprocal

(*The first three LengthUnit enumerated values already exist and are used with the Lattice object to distinguish between real space and reciprocal space quantities)

My idea for a quick implementation of this split-enumeration for interpolation value type identification would implement only some of the 15 combinations Now, reserving that others can be implemented in the Future (or Never): LengthUnit:: ValueType::vector ValueType::pseudovector ValueType::Gamma
::none Never (or default) Never (or default) Never (or default)
::angstrom Future Future #86
::inverse_angstrom Future Future Never
::real_lattice Now Now Now
::reciprocal_lattice Now Future Never

LengthUnit::none is currently used as a default unset value, and it may be sensible to extend this to the interpolation types as well. (E.g., ValueType::Gamma without a LengthUnit specified might be the new Cartesian eigenvector implementation.)

rebeccafair commented 1 year ago

Closed by #88, released in version 0.7.0