ACEsuit / ACE.jl

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

WIP: Multiple Properties codes #45

Closed andresrossb closed 3 years ago

andresrossb commented 3 years ago

Updating current linear codes to allow for multiple properties.

cortner commented 3 years ago

mutiple properties - ACE.jl doesn't know about species and doesn't want to know about species :)

cortner commented 3 years ago
   # stage 3: get the gradients
   fill!(g, zero(eltype(g)))
   for iX = 1:length(cfg)
      @inbounds @fastmath for iA = 1:length(basis1p)
         g[iX] += _real(dAco[iA] * dA[iA, iX])
      end
   end

might become something like

   # stage 3: get the gradients
   fill!(g, zero(eltype(g)))
   for iP = 1:num_properties, iX = 1:length(cfg)
      for iA = 1:length(basis1p)
         g[iP, iX] += _real(dAco[iA][iP] * dA[iA, iX])
      end
   end
cortner commented 3 years ago

For testing: check that evaluate, grad_config, grad_params, grad_params_config give same answers with

cortner commented 3 years ago

this ready to be merged?

cortner commented 3 years ago

we may need to rebase onto new main

cortner commented 3 years ago

@andresrossb Please pull this branch, I've rebased onto latest mater

cortner commented 3 years ago

I think we might be ready to review it tomorrow?

cortner commented 3 years ago

I think this is very close to merging, thanks for your work on this. Any questions for clarification, just add below the comments, or here.

cortner commented 3 years ago

Hi Andres - when would be a good moment to rebase your PR onto the latest MAIN branch? I'd like to do it asap so they don't diverge too much, but will wait until you have committed everything and don't work on it for a few minutes. Ping me in zulip?

cortner commented 3 years ago

please take another look at my questions and let's try and talk later

cortner commented 3 years ago

from my end the only thing left to do is add some documentation.

cortner commented 3 years ago

strange - a lot of tests are failing for me right now, I have no idea why

cortner commented 3 years ago

but I think this was caused in one of my PRs - I have no idea how I missed this.

cortner commented 3 years ago

most but not all tests are fixed. There is a nasty failure of grad_config_params cause by this:

function grad_params_config(m::LinearACEModel, cfg::AbstractConfiguration) 
   dB = acquire_dB!(m.basis, cfg)
   return [grad_params_config!(dB, m, cfg) for i = 1:length(m.c[1])]
end

Why was it necessary to wrap this in a vector? That makes its behaviour inconsistent with the scalar case and also inconsistent with what we discussed I thought?

I think this is the last thing we need to resolve before merging.

cortner commented 3 years ago

I fixed the FIO tests and merged into your branch. So now it should be possbile to get all tests to pass.

cortner commented 3 years ago

finally - thanks for your work. I won't delete the branch for now since we want to now look into the differentiation issue.