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

WIP: New RPI Coeff Implementation #17

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

WIP: New RPI Coeff Implementation #17

wants to merge 40 commits into from

Conversation

cortner
Copy link
Member

@cortner cortner commented Nov 13, 2024

Continuing from #16

@cortner cortner changed the title New RPI Coeff Implementation WIP: New RPI Coeff Implementation Nov 13, 2024
@cortner
Copy link
Member Author

cortner commented Nov 15, 2024

I extended the test suite to make it comprehensive. At the moment the tests fail when an (nn, ll) doesn't have any invariants at all, e.g., for

nn = [3, 3]
ll = [0, 2]

It seems your new code doesn't like this, but I think this needs to be allowed - one can't expect the user to do this filtering i.e. to know a priori what the zeros in the CG are?

@cortner
Copy link
Member Author

cortner commented Nov 15, 2024

I rewrite the test functions a bit so that the same random batch can be used for all tests so the spans can now be tested more easily.

I'll let you clean up the rest of the tests please.

Copy link
Member Author

@cortner cortner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall. Not ready to register in General but I can register it in ACE. Let me know which changes you want to make now, and those you don't want to make right away, please file issues.

Project.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,169 @@
# Alternative to the computation of rotation equivariant coupling coefficients
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is to become the new main implementation of O3, can you please move it to the main src folder and rename it O3.jl.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can hardly be done right now because O3_alternative can just provide the coupling coefficients for the complex spherical vector case. It is still missing how the CCs for the real spherical vectors can be obtained in this way. So I think we can retain both (or even do a merge?) for now? I will no matter what file an issue mentioning this.

test/new_rpe_test.jl Outdated Show resolved Hide resolved
test/new_rpe_test.jl Outdated Show resolved Hide resolved
@zhanglw0521
Copy link
Collaborator

zhanglw0521 commented Jan 24, 2025

It seems good to go to me.

In terms of O3 and O3_alternative - it should be filed as an issue, but I am also wondering if it could be a follow-up comment on Issue #3, which indeed concerns the speed of evaluating the coupling coefficients (maybe we will need to rename the issue though). What is left now is that we haven't yet had a faster evaluation of the CCs for the real equivariant vector, and once it has been implemented in O3_alternative, we can do a replacement and retire O3.jl.

Another reason why I think this might be a good compromise is that retaining both O3 and O3_alternative for now would allow anyone who uses EQM (if any...) not to change anything.

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