ignore checks for savenever plugins #345

JoranAngevaare · 2020-10-30T23:34:18Z

What is the problem / what does the code in this PR do
Fix #247, XENONnT/straxen#149 and prevent an implementation such as XENONnT/straxen#261.

Can you briefly describe how it works?
The inconsistency in time ranges may occur when we are loading plugins that are not saved themselves. In XENONnT/straxen#261 I proposed to solve this by making sure that all data was stored. However, @JelleAalbers pointed out that it might be actually nicer to fix this on the strax side.

Can you give a minimal working example (or illustrate with a figure)?
It took quite some trial and error to get it working. I propose to keep some of the associated Errors as Userwarnings to monitor if we are actually not making any edge case not working. I've tried whatever I could come up with but so far the data returned is exactly the same as the one I would get if we would store all the save_when.NEVER plugins (Peaks in this case).

How to replicate
I just ran the wf plot as in XENONnT/straxen#149 just until I hit this error, run the following two commands and you will see that you are now able to actually plot the wf:

# where time_range=(9343216270, 9343224420) is a time range in simulated data 
# where from I figured out that I could run into this bug
st.get_array(run_id, 'peaks', time_range=(9343216270, 9343224420))

WenzDaniel

I think it is fine, but I think we should monitor this change a bit.

JelleAalbers

Thanks for the fix Joran! I can remove the item "Fix the strax time range bug, it's only peaks, so just disable exhaustion check" from my todo list :-) I didn't understand the break in the input consolidation section yet.

JelleAalbers · 2020-11-10T04:14:29Z

strax/plugin.py

-                raise RuntimeError(
-                    f"Plugin {d} terminated with leftover {d}: {buffer}")
+                # This can happen especially in time range selections
+                if int(self.save_when) != strax.SaveWhen.NEVER:


Yes, nice, I think this is the key fix. (You could move the if condition up above the for loop for clarity, since the loop doesn't need to run at all for never-save plugins)

JelleAalbers · 2020-11-10T04:15:03Z

strax/plugin.py

+                        # for a time range that is inconsistent with the
+                        # chunking of the inputs. See:
+                        # https://github.com/AxFoundation/strax/issues/247
+                        break


I think this triggers even during regular processing of a run, without a time range selection. Are you sure it is harmless? Maybe I'm not understanding why this break is needed; did we get the "ten passes" error message when plotting waveforms?

Thanks, this was actually the first place I started looking. This was indeed not necessary and also felt slightly out of place. Fun fact, the input times were inconsistent (not the matter here).

I removed it now.

JoranAngevaare · 2020-11-26T10:25:31Z

Checkout

strax/tests/test_loop_plugins.py

Line 117 in d07cc14

# https://github.com/AxFoundation/strax/pull/345 (will fix in that

when reopening

JoranAngevaare · 2020-11-30T15:38:53Z

This PR has been validated in https://github.com/XENONnT/analysiscode/blob/master/StraxTests/Fix_time_for_savenever.ipynb and https://github.com/XENONnT/analysiscode/blob/master/StraxTests/Fix_time_for_savenever_validation.ipynb. I'm confident and happy that we can close this extremely long outstanding issue.

WenzDaniel · 2020-12-01T06:44:45Z

strax/plugin.py

+            # For non-saving plugins, don't be strict, just take whatever
+            # endtimes are available and don't check time-consistency
+            if int(self.save_when) == strax.SaveWhen.NEVER:
+                # </start>This warning/check will be deleted, see UserWarning


I assume you mean in a future upcoming release?

And you will just keep the end = max([v.end ....]) part right?

yes, that is true. I just added this warning to give a handle in case we start seeing shenanigans which I highly doubt at this point due to the tests I did.

WenzDaniel

Looks fine and I looked through your tests as well. It looks like you spend a lot of time in setting them up. So I was wondering, do you think it would be possible to boil down your "offending" peak tests into some unity test or similar? Maybe you have already thought about it.

JoranAngevaare · 2020-12-01T08:59:08Z

Looks fine and I looked through your tests as well. It looks like you spend a lot of time in setting them up. So I was wondering, do you think it would be possible to boil down your "offending" peak tests into some unity test or similar? Maybe you have already thought about it.

Thanks, actually, this should be hard because you basically need straxen. If I'd write a test it would take a lot of time because you'd be needing to copy over the chunks and set of a similar plugin-structure as in straxen but than with much more abstraction. In the end, the test would not be flexible.

As a trade-off I can try to see if I can somehow replicate with the demo data. If this is possible I would just move the test to straxen saving myself a lot of time.

Also, thanks to taking the time to look at the tests I did.

JelleAalbers

Very nice work!

ignore checks for savenever plugins

3c34338

JoranAngevaare mentioned this pull request Oct 30, 2020

Error in time range handling waveform plot XENONnT/straxen#149

Closed

JoranAngevaare requested review from WenzDaniel and JelleAalbers November 2, 2020 09:11

Merge branch 'master' into fix_timeselections

96be0a6

WenzDaniel approved these changes Nov 3, 2020

View reviewed changes

JoranAngevaare marked this pull request as draft November 5, 2020 16:10

JelleAalbers reviewed Nov 10, 2020

View reviewed changes

JoranAngevaare added 2 commits November 30, 2020 14:40

Merge branch 'master' into fix_timeselections

c50343f

remove superflous check

92e51b6

JoranAngevaare marked this pull request as ready for review November 30, 2020 15:37

JoranAngevaare requested review from WenzDaniel and JelleAalbers November 30, 2020 15:38

WenzDaniel reviewed Dec 1, 2020

View reviewed changes

WenzDaniel approved these changes Dec 1, 2020

View reviewed changes

reinstate runtimeerror for plugins whereto this PR does not apply

35b9507

JoranAngevaare mentioned this pull request Dec 1, 2020

Test time selection fix strax/#345 and update straxen tests XENONnT/straxen#298

Merged

JelleAalbers approved these changes Dec 1, 2020

View reviewed changes

JoranAngevaare merged commit 168924c into master Dec 1, 2020

JoranAngevaare deleted the fix_timeselections branch December 1, 2020 17:02

dachengx mentioned this pull request Apr 10, 2023

Set start of further chunk to be the smallest start of dependencies #715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ignore checks for savenever plugins #345

ignore checks for savenever plugins #345

JoranAngevaare commented Oct 30, 2020

WenzDaniel left a comment

JelleAalbers left a comment

JelleAalbers Nov 10, 2020

JelleAalbers Nov 10, 2020

JoranAngevaare Nov 30, 2020

JoranAngevaare commented Nov 26, 2020

JoranAngevaare commented Nov 30, 2020

WenzDaniel Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel left a comment

JoranAngevaare commented Dec 1, 2020 •

edited

Loading

JelleAalbers left a comment

ignore checks for savenever plugins #345

ignore checks for savenever plugins #345

Conversation

JoranAngevaare commented Oct 30, 2020

WenzDaniel left a comment

Choose a reason for hiding this comment

JelleAalbers left a comment

Choose a reason for hiding this comment

JelleAalbers Nov 10, 2020

Choose a reason for hiding this comment

JelleAalbers Nov 10, 2020

Choose a reason for hiding this comment

JoranAngevaare Nov 30, 2020

Choose a reason for hiding this comment

JoranAngevaare commented Nov 26, 2020

JoranAngevaare commented Nov 30, 2020

WenzDaniel Dec 1, 2020

Choose a reason for hiding this comment

WenzDaniel Dec 1, 2020

Choose a reason for hiding this comment

JoranAngevaare Dec 1, 2020

Choose a reason for hiding this comment

WenzDaniel left a comment

Choose a reason for hiding this comment

JoranAngevaare commented Dec 1, 2020 • edited Loading

JelleAalbers left a comment

Choose a reason for hiding this comment

JoranAngevaare commented Dec 1, 2020 •

edited

Loading