Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More flexible radial basis embedding #17

Merged
merged 18 commits into from
Oct 5, 2023
Merged

More flexible radial basis embedding #17

merged 18 commits into from
Oct 5, 2023

Conversation

zhanglw0521
Copy link
Collaborator

This PR aims to enable a more general radial embedding, as is addressed in Issue #11

@zhanglw0521
Copy link
Collaborator Author

Looks like this PR is ready to be reviewed. Now the input radial could be an abstract chain, along with its radial specification. The chain itself and its specification are stored in a new structure called Radial_basis (we can at any time change it). The only requirement to the chain is that it should map a configuration Vector{Positions,N_particle} to a $N_{particle} \times N_{invariant}$ matrix, which is rotation invariant.

@zhanglw0521
Copy link
Collaborator Author

Also, I enabled the rSH-based chain (for L=0 only) in this PR (This shouldn't have been in this PR but I saw rSH was used in forces.jl so it might do no harm to just add a few lines in this regard).

@zhanglw0521 zhanglw0521 requested a review from cortner September 30, 2023 04:02
@cortner
Copy link
Member

cortner commented Sep 30, 2023

Thank you, Liwei. @CheukHinHoJerry - can you do a first review please. I'll then take another look before merging.

@CheukHinHoJerry CheukHinHoJerry self-requested a review October 1, 2023 06:58
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
@CheukHinHoJerry
Copy link
Collaborator

Just to put a note here that 9321a06 resolved reverse over reverse of single species model.

@cortner
Copy link
Member

cortner commented Oct 3, 2023

@CheukHinHoJerry -- can you quickly confirm what you had to change?

Is it simply replacing the comprehension with a broadcast?

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Oct 3, 2023

Yes - more precisely, I changed from this:

Radial_basis(Chain(trans = WrappedFunction(xx -> [f_trans(norm(x)) * f_cut(norm(x)) for x in xx]),
                poly = lux(basis), ), spec)

to this:

Radial_basis(Chain(getnorm = WrappedFunction(x -> norm.(x)), trans = WrappedFunction(x -> f.(x)), poly = lux(basis), ), spec)

and similarly this also works:

Radial_basis(Chain(trans = WrappedFunction(x -> f.(norm.(x))), poly = lux(basis), ), spec)

Still, I don't understand fully the reason behind this yet.

@CheukHinHoJerry
Copy link
Collaborator

I used two work around, which should be taken care by ACEsuit/Polynomials4ML.jl#67 (comment) and #18

@cortner cortner merged commit 7f032f4 into main Oct 5, 2023
2 checks passed
@cortner cortner deleted the Rnl_Basis branch October 5, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants