search

ACEsuit / ACE.jl

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

Basis selectors extended #67

Closed MatthiasSachs closed 2 years ago

MatthiasSachs commented 2 years ago

Basis selectors for additional constraints implemenented. This includes 1) constraints described as sublevel-sets of a levelset function that is bounded from below by a degree function, and 2) arbitrary constraints, that don't need to satisfy any condition in regard to the degree function.

In particular, a basis selector for bond-environment basis functions was added.

cortner commented 2 years ago

@MatthiasSachs thank you so far for taking this on. Can you please get as much as possible of these fixes done. If in doubt, send me a message please. I will be done with meetings at 10 and will then take over finishing this PR.

cortner commented 2 years ago

@zhanglw0521 looks like the ideas are mostly there, just needs cleanup. Hopefully we can get this done. I'll ping you as soon as I think it is ready for you to use.

MatthiasSachs commented 2 years ago

One additional comment: I have the impression that the basis construction is relative slow now, and the resulting bases surprisingly large. I am not sure if this is correct or a bug, and it might be good idea to compare basis sizes against Christoph's and Liwei's implementation.

MatthiasSachs commented 2 years ago

One additional comment: I have the impression that the basis construction is relative slow now, and the resulting bases surprisingly large. I am not sure if this is correct or a bug, and it might be good idea to compare basis sizes against Christoph's and Liwei's implementation.

I think I fixed the bug. I guess the constructors for PNormSparseBasis and CategorySparseBasis are not very pretty/clean though as the user needs to provide exactly one of the optional arguments default_maxdeg and maxdeg .

cortner commented 2 years ago

is this ready for review?

MatthiasSachs commented 2 years ago

PR is ready for review. I commented out some code that would only work if made some changes in sparsegrids.jl (i.e., replace degree by level). Not sure why git thinks that there is conflict...?

cortner commented 2 years ago

so far I've just done the merging with main and some cleanup to make the code easier to read. I will next think about putting the "levelset" interface in. But we could decide to merge this now. However after the levelest and intersections are ready I think we need to reconsider these basis selectors. I can see them getting messier and messier.