search

ACEsuit / ACE.jl

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

Equiv #22

Closed MatthiasSachs closed 3 years ago

MatthiasSachs commented 3 years ago

New pull request:

First working version for EuclidianVector-basis

Relevant tests are provided in test/test_symmbasis-equiv.jl

The test sets is not yet added to the general test set.

cortner commented 3 years ago

thank you again! I'll review asap. Liwei is also almost ready, so it this is now getting exciting!

cortner commented 3 years ago

can we remove the dependency on ReferenceFrameRotations and just use randomly generated rotation matrices?

MatthiasSachs commented 3 years ago

Sure!

cortner commented 3 years ago

looks like basis generation is very slow, correct? we should hunt for some type instabilities

cortner commented 3 years ago

can you please give me push access to your repository then I can push some small changes. I just adpated your tests to the most recent changes in ACE.jl

cortner commented 3 years ago

but great thing is that all tests appear to pass ok!!

MatthiasSachs commented 3 years ago

I just added you as a collaborator to my forked version of the repository

cortner commented 3 years ago

I guess this is almost ready to merge. Just want to spend a bit more time reading through your changes. Thank you!

MatthiasSachs commented 3 years ago

With regards to the speed issue: I have not been focusing on speed yet. I am wondering if we could maybe more effectively make use of the orthogonality of the radial basis functions? If I recall correctly we are executing the "symmetrization operations" multiple times when we form the basis?

cortner commented 3 years ago

Correct but that’s also the case for invariants. This seems worse. But you are right we can push the performance back a a bit

cortner commented 3 years ago

I've made various comments. Sorry for cluttering this conversation, I should have created a single "review". Anyhow, would you be willing to take care of these points and push final changes? Then I'd probably like to have a conversation about merging also with Liwei's branch and in which order.

cortner commented 3 years ago

ok, I am merging this now, but see new items in #23