-
Notifications
You must be signed in to change notification settings - Fork 2
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
Comments
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. |
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. |
Let's have a chat sometime next week. |
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:
Old:
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.
The text was updated successfully, but these errors were encountered: