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

Update NaN filtering in InterpolatingMap #1302

Merged
merged 2 commits into from Dec 5, 2023

Conversation

JelleAalbers
Copy link
Contributor

@JelleAalbers JelleAalbers commented Dec 5, 2023

What does the code in this PR do / what does it improve?

Fixes InterpolatingMap so we do not crash with scipy 1.11.

Can you briefly describe how it works?

Scipy 1.11's kdtree no longer accepts nonfinite values (scipy/scipy#18502). However, we sometimes use NaN to represent missing/undefined values, e.g. for drift time if an event has no S1.

Currently we pass all points to InterpolatingMap, then force the result to NaN for points that have nonfinite distance to the neighbours the kdtree finds. That's fine in our current scipy 1.10 environment, but once we update (XENONnT/ax_env#353) there will be problems.

This PR instead checks for NaN before passing the points to InterpolatingMap to avoid crashing.

Can you give a minimal working example (or illustrate with a figure)?

See the new unit test.

I tried processing a small run 026195 locally with scipy 1.11.3, and it crashed here

delta_r = self.map(corr_pos)
where events with NaN drift time are being passed to a map. Now it no longer crashes.

@JelleAalbers JelleAalbers marked this pull request as ready for review December 5, 2023 13:00
@dachengx
Copy link
Collaborator

dachengx commented Dec 5, 2023

Thanks @JelleAalbers . Just for my education. How did you find this potential future crash?

@JelleAalbers
Copy link
Contributor Author

Thanks Dacheng -- I was processing a tiny run on my laptop, which meant all the package versions were different from the ones we usually use.

Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JelleAalbers .

@dachengx dachengx merged commit 3b0567b into XENONnT:master Dec 5, 2023
9 checks passed
@JelleAalbers JelleAalbers deleted the itpmap_nan branch December 6, 2023 07:29
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.

None yet

2 participants