search

lab-cosmo / librascal

A scalable and versatile library to generate representations for atomic-scale learning
https://lab-cosmo.github.io/librascal/
GNU Lesser General Public License v2.1
80 stars 18 forks source link

implement in structure_manager.hh a conditional get_distance that looks at the stack if any has the trait HasDistance is true. #86

Open felixmusil opened 5 years ago

agoscinski commented 5 years ago

this line looks wrong https://github.com/cosmo-epfl/librascal/blob/master/src/structure_managers/adaptor_increase_maxorder.hh#L56 I think should inherit from underlying manager like HasDirectionVectors

felixmusil commented 5 years ago

This is a bit delicate. the reason it is harcoded to false is because you don't have the distances for the new order but only the ones at order 2 if there were some. The inconsistency between HasDirectionVectors and Distance is a problem though.

In terms of usage, I am not sure if it is needed to acccess the distances between the atoms in a triplet/quadruplet/... ? Maybe @mastricker or @tjunge have a better idea about that ? if it is not, then we can leave the HasDistance as is and imply by it 'has pair distance' and make it consistent with HasDirectionVectors so that both get the value from the previous manager. Otherwise we would have to modify it so that there is one such flag per Order.

mastricker commented 5 years ago

I will need to think about that a little.

agoscinski commented 5 years ago

the original issue is solved in #130 see https://github.com/cosmo-epfl/librascal/blob/feat/get_property_forwarding/src/structure_managers/structure_manager.hh#L630-L643 https://github.com/cosmo-epfl/librascal/blob/feat/get_property_forwarding/src/structure_managers/adaptor_strict.hh#L249-L253

but the issue if AdaptorMaxOrder should have the trait HasDistance{false} is still open. Should I open new issue for this?