Change normalized to SphericalHarmonics and SolidHarmonics across all APIs
#137
Conversation
frostedoyster
force-pushed
the
new-api
branch
2 times, most recently
from
1ca11d3 to
82d6f60
Compare
frostedoyster
force-pushed
the
new-api
branch
3 times, most recently
from
eff52b4 to
4988f8d
Compare
…::CustomClassHolder`
|
I can't get torch to work, I think the inheritance |
|
Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping |
|
This would solve #98 |
Yes, you'll need multiple classes at the Torch API level, one for each calculator. |
|
Thanks, torch should be good. The docs are not ideal for now, but I'm planning to do a thorough docs revision PR after #139 is fixed |
| using sphericart_spherical_harmonics_calculator_t = sphericart::SphericalHarmonics<double>; | ||
| using sphericart_spherical_harmonics_calculator_f_t = sphericart::SphericalHarmonics<float>; | ||
| using sphericart_solid_harmonics_calculator_t = sphericart::SolidHarmonics<double>; | ||
| using sphericart_solid_harmonics_calculator_f_t = sphericart::SolidHarmonics<float>; |
There was a problem hiding this comment.
Do we really need separate types here? We could reduce the number of functions by either having a single calculator type, which is initialized to be either SphericalHarmonics or SolidHarmonics, or having two calculator types by a single function that can take both and call the right function on them.
If this is not an issue, we could also keep the two separate, but I feel like we have a lot of functions in the C API for no reason.
There was a problem hiding this comment.
I agree that we have a lot of functions (and we would have even more if we wanted to have a CUDA C API for e.g. CuPy). However, I feel like this is in line with the other APIs and the original idea behind this PR was to make the APIs as uniform as possible
There was a problem hiding this comment.
I agree that this is making the API a bit more uniform between C & the other languages, but I fear that this is also making the API really hard to use from pure C. We can decide that we don't care and I'd be fine with it, but otherwise I'd like to revise the C API before a 1.0 release.
This can still be merged for now, and we'll have another look later.
There was a problem hiding this comment.
Thanks. And yes, let's discuss in person
This is much more intuitive for the user (at the moment, if they ask for the spherical harmonics, they get something else by default), matches the Julia API, and also seems to be accepted in the field (see e3x API for spherical and solid harmonics)