-
Notifications
You must be signed in to change notification settings - Fork 13
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: Julia Implementation #79
Conversation
@tjjarvinen -- you might be interested in keeping an eye on this and look into translating the GPU kernels once the CPU kernels are merged. |
Thanks again for the PR @cortner!
I would also be happy to help with the documentation |
I've already tested against symbolics up to L=4 + confirmed that the basis is orthonormal. That implies that its the same as yours up to a sign.
yes I am. thanks for confirming. |
In Julia I would simply return derivatives as |
Thank you - that would be greatly appreciated. Even if just to document the differences initially. If the gap is bigger than expected then we can transfer your symbolic optimisations, or get more experienced Julia programmers to look at the reasons for the gap. |
If this is an SVector of dynamically allocated vectors, I don't think it would be equivalent (the SVector would probably contain pointers that are contiguous themselves, but which point to different memory blocks in the heap). In any case, we think it's much more important to fit within the idiomatic use different languages rather than keeping consistency across the APIs. Let's go with SVectors |
Regarding the benchmarking, here is what worked for me:
This builds |
I'm getting, for l=6 and 10000 samples,
on my laptop with a lot of other stuff going on (so the results are probably not very reliable). In any case, the speed of this implementation looks already very good. |
Dont worry - I would never form a vector of dynamically allocated vectors, that's madness. I'll explain the memory Layout with SVectors later when I have time. Thanks also for the timings and the advise on how to run benchmarks myself. I'll try to reproduce locally and on my groups server. |
Id very much like to understand how you get that dort if timing for a generic implementation that doesn't that doesn't do static analysis and unrolling. Or does it?maybe your generic is closer to my generated and your generated is a bit of extra optimisation of the symbolic expressions on top? |
Our generic implementation is as you say: it doesn't assume anything. Instead, our hardcoded expressions up to l_max=6 are all templated, so that l_max is static and the loops should therefore be unrolled. If l_max>6, we split all loops into one that goes from 0 to 6 (unrolled) and one that goes from 7 to l_max (not unrolled). Based on this, your implementation might be faster than ours for l_max>6. I'd like to try, but I'm having issues at the moment on our clusters and I don't trust my laptop. It will be interesting to see the results on your clusters! |
In other words, we're tying together hardcoding and templating l_max for simplicity. If the Julia implementation is faster than the C++ one for l_max>6, it might be worth templating our l_max>6 spherical harmonics (even though they're not hardcoded) and see what happens |
Very nice idea - I will add this. |
My laptop timings are completely inconsistent with those in our paper -- I wouldn't trust them. I'll try to get actual timings when they fix my cluster account, hopefully on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are couple of things I noticed related to FLoat32 calculations.
I've finally managed to run some benchmarks on our cluster. Here are the results
|
This looks about right. On my machine it's
If I generate optimized polynomial expressions symbolically I think I gain maybe another 30-40% performance. We could do this later, but for now I want to stick with this automatically generated version until it's finished, tested etc. |
gradients for batched evaluation was done. When reading your paper I was very sceptical about your gradient timings, I thought they were poor. But I didn't want to say anything until I tried myself, I think while my evaluation is a bit slower than yours, my gradients are actually faster.
|
An interesting follow-up - my code-generated gradient have a similar performance loss (factor 4) as your "generic codes" - which I think means my generated codes must be quite similar in spirit to your generic codes.
|
Maybe I should briefly explain Julia SVectors so the format how I return gradients is understood. In Julia, a Further, if I generate a vector of svectors, i.e. an object of type
then this will be allocated on the heap, but because each element |
Ok, no problem. In my opinion, as long as the documentation is there in the readme, we can take the responsibility to port it to readthedocs in a PR in the near future, ideally with your review to make sure that the resulting documentation is correct. @Luthaf I would really like to hear your opinion as well here |
I also have no problem transferring the README now to readthedocs, and add a link in the README to the documentation. But I don't want it twice. I don't know how to have the nice automatic doc generation though. Maybe that part could be postponed? |
Regarding readthedocs, I don't think they have a good integration with Documenter.jl, which is a bit sad. We could use github pages to host documentation instead, but we would loose documentation preview on PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small point we can also resolve later. I don't think having the code in a folder named Sphericart.jl
is required for registration. Given we have the python code in python
directory, I would prefer to have this code in a julia
directory at the root.
SpheriCart.jl/Project.toml
Outdated
@@ -0,0 +1,19 @@ | |||
name = "SpheriCart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I've always considered the package to be a single word, not a concatenation of two of them
name = "SpheriCart" | |
name = "Sphericart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll only accept that change under pressure. It is clearly a concatenation of words and hence the C should be capitalised. It is not an "Eigenname".
SpheriCart.jl/README.md
Outdated
the published paper and without consulting the reference implementation | ||
provided by the authors. | ||
|
||
`SpheriCart.jl` is released under MIT license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`SpheriCart.jl` is released under MIT license. | |
`Sphericart.jl` is released under both MIT and Apache license. |
This should not be left for later. It would be huge pain to change the registry manually since it would require manual merging from a maintainer. I don't like this change either - but will accept it under sufficient pressure. |
I would hope that the name of the source directory in the git repo where a given project can be found is part of the project version in the registry, and can be changed easily from one version to the next, but I'll check this. Do you know if there is an equivalent to Re name and folder change, I'll ask for more opinions then. @frostedoyster and @ceriottm: should the package be called |
As I said above, it requires a manual PR and a manual merge from a highly overstreched group of maintainers. |
If there is then I don't know about it. Julia packages tend to just add a little button on their github frontpage that reports the CI status. (tests passing/failing) |
Regarding the naming : the Julia convention is very clear on this
regarding the folder name: I'm not thrilled with the change, it makes me uncomfortable for some reason, but I don't have any strong argument to not change it. |
|
How is this different from CI? Julia gets installed, the package gets installed, the dependencies get installed (including artifacts) and then the tests are run. |
|
None of this happen on It also seems that the sub directory is not recorded with the version (cf https://github.com/JuliaRegistries/General/blob/244be981c9030ee146c070d56811e02721e403ae/C/cuDNN/Package.toml#L4, this is not in Versions.toml), meaning that this is something we need to decide on now. Same for the package name, we need to decide before registering it. |
This would be the registrator bot. But it works a bit differently. In fact it is much more stringent I think. It will only merge the registration request automatically if all tests pass. |
No super-strong feelings here but my preference would be:
|
Closing the pull request was a mistake, we're not sure how that happened. So, @cortner, we can rename the folder to |
No problem. Please take a look at the existing documentation in the README and tell me if you think it needs to be significantly expanded? |
I would say that we need at least a mention (or perhaps an example?) of |
It looks like the renaming of the folder caused some issues with the tests. I'm looking into it, should be fixed relatively soon |
Thank you very much for all this work @cortner! We might need your review later when we merge the documentation with our own |
This PR implements a Julia package
SpheriCart.jl
as discussed in #23. Note this is MIT licensed. In the API I tried to mimic the Python interface but adapt it to Julia style. The low-level interface is more C like but also using some Julia conveniences. I could get rid of that if desired.Done so far:
README.md
TODO to merge PR:
For the last point, it may be necessary to fix my basis ordering. I haven't yet looked into what ordering you use. But it would be a trivial change.EDIT: it seems the ordering is fine.TODO FUTURE:
I propose that "TODO" need to be finished to merge this but the "TODO FUTURE" points can be added in future PRs.