Trying to fix touching_windows bug

[FaroutYLq]

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What is the problem / what does the code in this PR do
Currently pema is not usable because of a bug shown in strax.touching_windows here. See here for the issue. This PR tries to understand what happened and bypass the bug.

Can you briefly describe how it works?
It seems to be a numba problem that, the _check_objects_non_negative_length will lead to problems, even though there is no negative length at all.

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

Please include the following if applicable:

• Update the docstring(s)
• Update the documentation
• Tests to check the (new) code is working as desired.
• Does it solve one of the open issues on github?

Please make sure that all automated tests have passed before asking for a review (you can save the PR as a draft otherwise).

[coveralls]

coverage: 97.622% (-0.2%) from 97.815% when pulling cb59e4a on tw_fix into 1129a79 on master.

[FaroutYLq]

I did a dirty workaround to make pema runnable again (it is passing the checks again). However, I don't think I understand why the issue can happen to begin with. It seems numba related hack.

[dachengx]

Maybe we can also try to sort the windows and things first, and finally sort them back, like:
https://github.com/XENONnT/straxen/blob/6bddda5679ecb5cb2c718912242173c24638e989/straxen/plugins/peaks/peak_ambience.py#L77-L82

[FaroutYLq]

Maybe we can also try to sort the windows and things first, and finally sort them back, like: https://github.com/XENONnT/straxen/blob/6bddda5679ecb5cb2c718912242173c24638e989/straxen/plugins/peaks/peak_ambience.py#L77-L82

I am not sure why this check non-negative length is related to sorting? Can you please explain a bit more?

[dachengx]

I am not sure why this check non-negative length is related to sorting? Can you please explain a bit more?

nvm, I was wrong.

[JYangQi00]

Thanks Lanqing and Dacheng,

After reviewing the PR with Lanqing, we realized a few points which I will summarize here:

• There is a strange issue with _check_objects_non_negative_length in which regular python will reach an overflow error but numba will not. (See here: https://github.com/AxFoundation/strax/pull/756)
• There is another strange issue in WFSim in which the t_last_photon in truth is NaN. There doesn't seem to be a discernible pattern as to why this is, and it seems very much to be a WFSim issue.
• The endtime of truth in the MatchPeaks's compute method is then set to t_last_photon which will be negative via conversion from a NaN
• This PR bypasses the _check_objects_non_negative_length, by using strax.general._touching_windows directly which uses peaks as a container for truth. Since peaks doesn't have any timing issues but truth does, the truth entries with negative endtimes will simply not be counted in the window. In this sense, this PR does seem to bypass the issue. However, I do not know what strange efficiency miscalculations could arise from this

[FaroutYLq]

Thanks @JYangQi00 for the summary. I am trying to improve a bit. When this negative length thing happen (which is quite often in our current wfsim), we manually overwrite the problematic truth['event_number'] to be -1, for all truth peaks in that simulation. By doing that we will just simply filter out negative event number from any analysis, and it will not happen that "some simulated event has only S2 matched but not S1" because of this wfsim negative endtime issue.

[FaroutYLq]

I am planning to make a pema release so that we don't get annoyed by the failed checks in base_environment. Any concern here?

[dachengx]

I am planning to make a pema release so that we don't get annoyed by the failed checks in base_environment. Any concern here?

after this merged?

[FaroutYLq]

Yes On Sep 11, 2023, at 22:39, Dacheng Xu ***@***.***> wrote: I am planning to make a pema release so that we don't get annoyed by the failed checks in base_environment. Any concern here? after this merged? — Reply to this email directly, view it on GitHub<#333 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALG57AUP4QPWWT3SYN2CGE3XZ5ZITANCNFSM6AAAAAA4SIC7MA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[FaroutYLq]

Sorry I meant pema release. Not new tag version of base_environment.

[JYangQi00]

Instead of finding the bad event numbers and looping, can't we just do allpeaks1[allpeaks1['endtime']<0]=-1?

[FaroutYLq]

Thanks. The idea why I didn't do this is because the following imaginary case. There is a peak in one simulated event, either S1 or S2, turns out having negative length by mystery, we wanted to make sure both S1 and S2s from that event has been tagged "bad". Otherwise, later in analysis by filtering out those bad event_number==-1, you will have S1 or S2 only event, which is additionally confusing.

[JYangQi00]

I see, thanks! Then we can merge

[JYangQi00]

Hi Lanqing, everything looks good to me and I think we can merge, but I think it may be more efficient to make the suggested change