Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Try using scipp.elemwise_func #381

wants to merge 3 commits into from

Conversation

SimonHeybrock
Copy link
Member

Considering some options for using scipp.elemwise_func. Please see comments in code.

@SimonHeybrock
Copy link
Member Author

@jl-wynen Do you have any preferences on how to proceed?

src/scippneutron/conversion/tof.py Outdated Show resolved Hide resolved
src/scippneutron/conversion/tof.py Outdated Show resolved Hide resolved
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?
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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.

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?
Copy link
Member

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.

Copy link
Member Author

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)?

Copy link
Member

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.

src/scippneutron/conversion/tof.py Show resolved Hide resolved
@jl-wynen
Copy link
Member

jl-wynen commented Dec 8, 2022

Looks reasonable. Except, we maybe shouldn't use else with try and group all kernels in there but use something like if numba_availale: ... else: ...

@SimonHeybrock
Copy link
Member Author

Looks reasonable. Except, we maybe shouldn't use else with try

Why?

@jl-wynen
Copy link
Member

jl-wynen commented Dec 8, 2022

Why?

  • To group the implementation of the numba kernel with the outer python function. When there are multiple of each, it gets harder to read if the numba kernels are all at the top of the file and the others scattered throughout.
  • Minor: To avoid try-else (I don't know the precise semantics)

@SimonHeybrock
Copy link
Member Author

  • Minor: To avoid try-else (I don't know the precise semantics)

I think it is actually recommended? It avoids putting more than necessary into the try:-block, thus avoiding catching more things than intended.

@jl-wynen
Copy link
Member

jl-wynen commented Dec 8, 2022

But I was suggesting

try:
  import numba
except ImportError:
  numba = None

[...]

if numba is not None:
  def kernel():
    ...
else:
  kernel = None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants