-
Notifications
You must be signed in to change notification settings - Fork 3
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
Try using scipp.elemwise_func
#381
base: main
Are you sure you want to change the base?
Conversation
@jl-wynen Do you have any preferences on how to proceed? |
src/scippneutron/conversion/tof.py
Outdated
return np.NAN if delta_tof <= 0.0 else dE(tof, t0, scale, incident_energy) | ||
|
||
# Converting everything to float64... | ||
# could make a different branch for float32? |
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.
Branch? We would need to extend the C++ implementation, no?
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.
Yes, and branch here to compile two difference numba.cfuncs
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.
Is there a practical need for single precision floats in this case?
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.
Could be, inelastic data can be huge.
src/scippneutron/conversion/tof.py
Outdated
return func(tof, t0, scale, incident_energy) | ||
|
||
# Should we keep this to continue support if the user does not have numba? | ||
# Or should numba become a mandatory dependency of scippneutron? |
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.
For now we should keep both because numba is a pretty big dependency. But we need a way to select which implementation is used in order to test both.
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 way would be to use tox
, with two different environments (with and without numba)?
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.
Yes. This would be expensive but thorough because it would ensure that everyhing works without numba.
Looks reasonable. Except, we maybe shouldn't use |
Why? |
|
I think it is actually recommended? It avoids putting more than necessary into the |
But I was suggesting try:
import numba
except ImportError:
numba = None
[...]
if numba is not None:
def kernel():
...
else:
kernel = None |
Considering some options for using
scipp.elemwise_func
. Please see comments in code.