search

ACEsuit / ACE.jl

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

enable scaling function in ACE main branch #49

Closed zhanglw0521 closed 3 years ago

zhanglw0521 commented 3 years ago

Enable scaling function for PiBasis & SymmetryBasis in branch main

zhanglw0521 commented 3 years ago

Seems that it is ready to be merged now. Things left in my mind are:

Thanks.

cortner commented 3 years ago

@zhanglw0521 can you please merge main into your branch and then make sure everything is still compatible and still passes?

zhanglw0521 commented 3 years ago

@zhanglw0521 can you please merge main into your branch and then make sure everything is still compatible and still passes?

Sure

cortner commented 3 years ago

also take a good look at the new basis selector interface please. this could be a very nice way to control the polynomial degrees for the environment atoms in the TB models. And this in turn will much better control the basis size and hence improve generalization.

zhanglw0521 commented 3 years ago

No problem, thank you.

cortner commented 3 years ago

this looks good now and I'm happy to merge it. I'm just finally wondering whether there is any sort of test we could implement? To test the "correctness" of the implementation? Any thoughts?

zhanglw0521 commented 3 years ago

I also concern about it... For the scalar basis (invariant), I could check it manually for small maxdeg(s), ord(s), etc. But I have no idea how to check the correctness in general, especially for vector/matrix basis.

cortner commented 3 years ago

Have you checked it manually with the current implementation? Then I’m happy for now. If there are bugs then they will float to the surface. If you can come up with a test later we can add it.

cortner commented 3 years ago

So if this is the last issue then as soon as you confirm I will squash+merge.

zhanglw0521 commented 3 years ago

I just checked the result for maxdeg = 3, ord = 1,2, (edited: p = 1,2), manually, and it returned the desired results (provided that I understand the scaling function correctly). Of course these are the simplest cases only so maybe a reasonable test is still needed in the future. Or would it be better for me to add documentation to the scaling functions?

cortner commented 3 years ago

I'd like to move on with this for now so I can tag a version for Matthias and for James. Let's add documentation a little later when we've converged the right interface for the scaling function.