ACEsuit / ACE.jl

Parameterisation of Equivariant Properties of Particle Systems
65 stars 15 forks source link

Fix for LinearACEModel with mutiple EuclideanVector or EuclideanMatrix properties #144

Closed MatthiasSachs closed 1 year ago

MatthiasSachs commented 1 year ago

See the example code + comments below for a description of the bug/problem.

using ACE
using Random
using StaticArrays
Bsel = ACE.SparseBasis(; maxorder=2, p = 2, default_maxdeg =5) 
r0 = .4
RnYlm = ACE.Utils.RnYlm_1pbasis(;  r0 = r0, 
                                rin = .5*r0,
                                trans = PolyTransform(2, r0), 
                                pcut = 1,
                                pin = 2, 
                                Bsel = Bsel, 
                                rcut=1.0,
                                maxdeg=5
                            );

Zk = ACE.Categorical1pBasis([:Cu,:H]; varsym = :mu, idxsym = :mu) 

#Invariant case

basis_inv =  ACE.SymmetricBasis(ACE.Invariant(Float64), RnYlm*Zk, Bsel;);

# Linear model for multiple invariant properties works, i.e.,
c = rand(SVector{2,Float64},length(basis_inv))
ACE.LinearACEModel(basis_inv,c)

# For covariant properties 
basis =  ACE.SymmetricBasis(ACE.EuclideanVector(Float64), RnYlm*Zk, Bsel;);
# a Linear model with a single property works 
c = rand(length(basis))
ACE.LinearACEModel(basis,c)
# but an error is thrown if we consider multiple properties, i.e.,
c = rand(SVector{2,Float64},length(basis))
ACE.LinearACEModel(basis,c)

Appropriately extending the multiplication operation in properties.jl as

*(prop::ACE.EuclideanVector, c::SVector{N, T}) where {T<:Number,N} = SVector{N}(prop*c[i] for i=1:N)
*(prop::ACE.EuclideanMatrix, c::SVector{N, T}) where {T<:Number,N} = SVector{N}(prop*c[i] for i=1:N)

fixes this issue though I am not sure whether this would be the preferred way of resolving this bug. The above two lines are added in this pull request,

MatthiasSachs commented 1 year ago

This PR request will need an update once PR #145 is merged into main.

MatthiasSachs commented 1 year ago

@cortner I think this PR is now ready to be merged.