Fixing EuclideanVector

[cortner]

A bunch of bugs in Euclidean Vector

• real never worked
• gradients of LinearACEModel didn't evaluate at all
• some allocations were missing

This is now at the stage where EuclideanVector works again, but now multi-properties fails. Further, I'd like to use this PR to also redesign EuclideanVector to allow it to take a real value.

[cortner]

@MatthiasSachs This is done now. I'd be very grateful if you'd be willing to review it.

[cortner]

@andresrossb If you have time to look at the multi-property aspects, I'd appreciate that since I've changed those around as well.

[cortner]

hm ... something is off I might have pushed to the wrong branch... Hold off on the review.

[cortner]

@MatthiasSachs @andresrossb should be fine now. sorry about the noise

[MatthiasSachs]

Seems all fine to me. There is no need for complex EuclidianVectors, right?

[cortner]

well - there is because we need them for the coupling coefficients. And this implementation should still allow you to construct complex output vectors as well, but this is entirely untested and indeed might never be needed.

[andresrossb]

as The derivative tests in ACEgnns are passing. Is there anything more specific I should look at?

[cortner]

However, there is also a trick here -- contracting a complex invariant of euclidean tensor effectively gives us two properties for the price of one :)

[cortner]

The derivative tests in ACEgnns are passing. Is there anything more specific I should look at?

I think that's the most important thing. Other than that only if you are interested in looking at how I've changed the code we worked on.

[cortner]

If you are all happy, then I'll probably tag another version sometime today

[MatthiasSachs]

well - there is because we need them for the coupling coefficients. And this implementation should still allow you to construct complex output vectors as well, but this is entirely untested and indeed might never be needed.

I am asking because of isrealB(::EuclideanVector) = true. Should we make that something like isrealB(::EuclideanVector{T}) = (T <: Real ? true : false) ?

[MatthiasSachs]

Oh, and the <: Real in

struct EuclideanVector{T} <: AbstractProperty where T <: Real
   val::SVector{3, T}
end

must also go, right? Do you want me to do these changes and add a test?

[cortner]

I thought I had fixed that.

[cortner]

not sure what you are seeing - this is what I have:

isrealB(::EuclideanVector{T}) where {T} = (T == real(T))
isrealAA(::EuclideanVector) = false

[cortner]

and the constraint on the property seems to have had no effect at all?

[cortner]

all good now?

[MatthiasSachs]

OK! Sorry, there was some issue with the version I viewed locally. I double checked for the constructor, but forgot to do the same for the isrealB issue...

[cortner]

well, all tests pass so I'll go ahead and merge. If we find bugs we return ...