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.

Sign up for GitHub

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

ignore checks for savenever plugins #345

Merged
merged 5 commits into from
Dec 1, 2020
Merged

ignore checks for savenever plugins #345

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c34338
ignore checks for savenever plugins
JoranAngevaare Oct 30, 2020
96be0a6
Merge branch 'master' into fix_timeselections
JoranAngevaare Nov 2, 2020
c50343f
Merge branch 'master' into fix_timeselections
JoranAngevaare Nov 30, 2020
92e51b6
remove superflous check
JoranAngevaare Nov 30, 2020
35b9507
reinstate runtimeerror for plugins whereto this PR does not apply
JoranAngevaare Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions strax/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import logging
import time
import typing

from warnings import warn
from immutabledict import immutabledict
import numpy as np

Expand Down Expand Up @@ -345,13 +345,21 @@ def iter(self, iters, executor=None):
self.input_buffer[d].split(
t=this_chunk_end,
allow_early_split=True)

# If any of the inputs were trimmed due to early splits,
# trim the others too.
# In very hairy cases this can take multiple passes.
# TODO: can we optimize this, or code it more elegantly?
max_passes_left = 10
while max_passes_left > 0:
if int(self.save_when) == strax.SaveWhen.NEVER:
# So this is a bit funny, this is the only time we may
# be actually running a plugin that is not saved with
# the time range. This means that we might be asking
# for a time range that is inconsistent with the
# chunking of the inputs. See:
# https://github.com/AxFoundation/strax/issues/247
break
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


this_chunk_end = min([x.end for x in inputs.values()]
+ [this_chunk_end])
if len(set([x.end for x in inputs.values()])) <= 1:
Expand Down Expand Up @@ -409,8 +417,10 @@ def cleanup(self,
# Check the input buffer is empty
for d, buffer in self.input_buffer.items():
if buffer is not None and len(buffer):
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:
Copy link
Member

Choose a reason for hiding this comment

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

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)

warn(f"Plugin {d} terminated with leftover {d}: {buffer}",
RuntimeWarning)

def _check_dtype(self, x, d=None):
# There is an additional 'last resort' data type check
Expand Down Expand Up @@ -452,10 +462,27 @@ def do_compute(self, chunk_i=None, **kwargs):
if len(kwargs):
# Check inputs describe the same time range
tranges = {k: (v.start, v.end) for k, v in kwargs.items()}
if len(set(tranges.values())) != 1:
raise ValueError(f"{self.__class__.__name__} got inconsistent "
f"time ranges of inputs: {tranges}")
start, end = list(tranges.values())[0]

# 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you mean in a future upcoming release?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if len(set(tranges.values())) != 1:
end = max([v.end for v in kwargs.values()]) # Don't delete
message = (
f"New feature, we are ignoring inconsistent the "
f"possible ValueError in time ranges for "
f"{self.__class__.__name__} of inputs: {tranges}"
f"because this occurred in a save_when.NEVER "
f"plugin. Report any findings in "
f"github.com/AxFoundation/strax/issues/247")
warn(message, UserWarning)
# This block will be deleted </end>
elif len(set(tranges.values())) != 1:
message = (f"{self.__class__.__name__} got inconsistent time "
f"ranges of inputs: {tranges}")
raise ValueError(message)
else:
# This plugin starts from scratch
start, end = None, None
Expand Down