scipp / scippneutron

Neutron scattering toolkit built using scipp for Data Reduction. Not facility or instrument specific.
https://scipp.github.io/scippneutron/
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

Should scippneutron stay a C++ library, or move to Python-only? #342

Closed SimonHeybrock closed 1 year ago

SimonHeybrock commented 2 years ago

Since this comes up periodically I thought I would gather the arguments here. I am not arguing for either right now, this is mainly for future reference.

Reasons to change to Python-only

Reasons to keep the C++ dependence on scipp

Coordinate transformations

Regarding coordinate transformations, I want to analyze a couple of cases. The relevant ones are those where more than one operation with a "large" operand are needed. I am excluding things such as two_theta. Those may be large when gravity-correction is used, but that is a special case and has less relevance.

tof -> energy

https://github.com/scipp/scippneutron/blob/31b2e6541d0d40ce11ec40335e6ddf315f0cfe93/src/scippneutron/core/conversions.py#L102-L105

tof -> energy transfer direct

https://github.com/scipp/scippneutron/blob/31b2e6541d0d40ce11ec40335e6ddf315f0cfe93/src/scippneutron/core/conversions.py#L108-L116

SimonHeybrock commented 2 years ago

We may consider reviving scipp/scipp#1898 for this. So far we have not done a performance test with this though, so it is not clear if it would provide the required speed.

nvaytet commented 2 years ago

Do we really expect these unit conversions to be the bottleneck of the workflows? I guess this is what you meant by "I want to analyze a couple of cases". If we consider this in the wider context of the entire workflow, it may not be so important if this step is slower, if other parts of the workflow take longer anyway?

The memory consideration (all the extra allocations) is probably the more important one. Do I understand correctly that a custom Numba kernel could allow to do this with only a single allocation? If so, then maybe it would be a good alternative even if the conversion is slower than native C++?

SimonHeybrock commented 2 years ago

Do we really expect these unit conversions to be the bottleneck of the workflows?

Probably not, but I think it would definitely lower the point at which performance becomes an annoyance to users.

jl-wynen commented 2 years ago

Do I understand correctly that a custom Numba kernel could allow to do this with only a single allocation?

Yes. Only one allocaction on our side. No idea what numba does, though. But it would definitely reduce the memory requirements of coordinate transforms. However, we might be able to express them in such a way that that is a non-issue. See Simon's older comment.

SimonHeybrock commented 1 year ago

I think to support coming to a decision here we should make a benchmark, comparing (for our most complex conversion step, probably inelastic scattering?):

SimonHeybrock commented 1 year ago

I'll close this, as the ongoing release should address this (making scippneutron Python-only).