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

Implementation of CG coefficients #13

Open
TSGut opened this issue Nov 10, 2023 · 3 comments
Open

Implementation of CG coefficients #13

TSGut opened this issue Nov 10, 2023 · 3 comments

Comments

@TSGut
Copy link

TSGut commented Nov 10, 2023

I was going to suggest a different implementation for CG but someone else already made a very robust one at https://github.com/Jutho/WignerSymbols.jl

I have some ideas and examples that show it can be done faster (up to 10x faster in many cases) but those involve risking instabilities in some edge cases and since the CG coefficients are pre-cached in applications it won't be worth the headache of ensuring those are dealt with separately. WignerSymbols.jl seems like a pretty small and mature package, it basically just receives updates to keep up with version level changes, and we get speed ups of up to factor 2 without risking instability.

Here are some benchmarks:

New:
image

Old:
image

The syntax is basically equivalent to the ACE one with a minor difference that we could overload, thus requiring no changes in other packages. I don't have strong feelings either way but since I have been exploring and testing CG and Wigner D, this seemed worth pointing out. If it's desired at any point, I am happy to make a PR and add the consistency tests.

@cortner
Copy link
Member

cortner commented Nov 10, 2023

I don't mind replacing our CG with their. @zhanglw0521 should be asked since he is currently maintaining this package.

Long-term I'm not convinced it's the right approach since we want to move towards automated generation of CGs, higher-order CGs and where possible also the reps.

@zhanglw0521
Copy link
Collaborator

zhanglw0521 commented Nov 10, 2023

I don't have many concerns about implementing other versions of Clebsch-Gordan coefficients as long as it is tested to be faster and 100% consistent. Actually I am aware of another package that does a similar thing but I haven't benchmarked anything yet.

I think the reason why we (or myself) insisted on the ACE version was just that our recursion formulae were all based on the ACE version and it feels safer to me to use the version that we are familiar with. After all, the CGs were pre-cached so perhaps correctness (or say consistency) would make more sense to me instead of efficiency (and also we might retire the old version in the future)?

Anyway, I am open to this discussion.

@cortner
Copy link
Member

cortner commented Nov 10, 2023

Let's have a chat sometime next week.

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

No branches or pull requests

3 participants