search

ACEsuit / ACE.jl

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

fixed adjoint_EVAL_D #53

Closed andresrossb closed 3 years ago

andresrossb commented 3 years ago

fixed adjoint_EVAL_D. It now also works with multiple properties. Needs testing.

cortner commented 3 years ago

I'm nervous about this branch - it seems this is based on an early 0.12 version? A lot has happened since then ... maybe ok, but we should merge main back in asap. I'll actually do this before fixing the tests.

cortner commented 3 years ago

after merging in main, all tests now pass. Can you take another look to see what changed?

cortner commented 3 years ago

Andres: "I finished the test for the adjoint in ACE. It is now ready to merge. I tested it against grad_params_config for single property, which I understand has tests elsewhere"

cortner commented 3 years ago

This has a few significant changes/additions so I want to take a close look before merging. I hope that's ok

cortner commented 3 years ago

@andresrossb -- my memory is we agreed to add some tests before merging? Is that correct?

andresrossb commented 3 years ago

@andresrossb -- my memory is we agreed to add some tests before merging? Is that correct?

yes, I also had to do some changes to the evaluation function and genmul! so I think it would be better to wait until we have a discussion about it. I implemented the adjoint_EVAL_D test, but I'm still missing the loss function test.

cortner commented 3 years ago

looking good. I think we need a review and a cleanup, then test that all this works with you ACEflux codes and then we are ready to merge?

cortner commented 3 years ago

!! ready to go finally !!