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

Use warning also in abs_time_to_prev_next_interval #738

Merged
merged 2 commits into from Jul 6, 2023

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Jun 29, 2023

What is the problem / what does the code in this PR do

Sometimes things in abs_time_to_prev_next_interval might be overlapping, or the endtime might be not sorted. So throw warning instead of error, following the logic of touching_windows.

Can you briefly describe how it works?

Just change the warning logic, the algorithm is not changed.

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

@dachengx
Copy link
Collaborator Author

If my understanding is correct, the abs_time_to_prev_next_interval will return a valid result if endtime of things are sorted, but not things are not overlapping.

@dachengx dachengx requested a review from WenzDaniel June 29, 2023 14:29
@coveralls
Copy link

Coverage Status

coverage: 91.546% (-0.05%) from 91.591% when pulling b66172b on abs_time_to_prev_next_interval_warning into bf0c16b on master.

Copy link
Collaborator

@FaroutYLq FaroutYLq left a comment

Choose a reason for hiding this comment

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

Looks good to me. How often do you expect this warning be triggered? I won't delete log files for this round of reprocessing so we can trace back anywise

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 6, 2023

Looks good to me. How often do you expect this warning be triggered? I won't delete log files for this round of reprocessing so we can trace back anywise

For this function, I think you will not see warnings in straxen's plugins(because it is only used on the event level in straxen). But in some customized plugins outside strax(en), it is better to print out the warnings rather than errors just like touching_windows and split_by_containments.

@FaroutYLq
Copy link
Collaborator

For this function, I think you will not see warnings in straxen's plugins(because it is only used on the event level in straxen).

Sorry I am confused, why I won't see these warning? For reprocessing we goes up to events

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 6, 2023

Sorry I am confused, why I won't see these warning? For reprocessing we goes up to events

Events in straxen are not overlapping so that they will not trigger warnings.

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 6, 2023

Events in straxen are not overlapping so that they will not trigger warnings.

So this PR is almost only for the users applying the function to their own data where the things might be overlapping.

@FaroutYLq
Copy link
Collaborator

Events in straxen are not overlapping so that they will not trigger warnings.

Wait then I think I need to reevaluate this PR: if this abs_time_to_prev_next_interval is only used in event level, and in event level we don't expect any overlap, should we keep the original assertion back there?

@FaroutYLq FaroutYLq self-requested a review July 6, 2023 17:44
@dachengx
Copy link
Collaborator Author

dachengx commented Jul 6, 2023

Wait then I think I need to reevaluate this PR: if this abs_time_to_prev_next_interval is only used in event level, and in event level we don't expect any overlap, should we keep the original assertion back there?

abs_time_to_prev_next_interval is used in straxen event level. But it can also be applied to other levels or other data, outside straxen. strax is an independent package from any specified TPC so we would allow users to apply it to any kind of data?

@FaroutYLq
Copy link
Collaborator

abs_time_to_prev_next_interval is used in straxen event level. But it can also be applied to other levels or other data, outside straxen. strax is an independent package from any specified TPC so we would allow users to apply it to any kind of data?

It is true that strax is an independent package... Then should we add no-overlap assertion, correspondingly in straxen where abs_time_to_prev_next_interval is applied?

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 6, 2023

Wait then I think I need to reevaluate this PR: if this abs_time_to_prev_next_interval is only used in event level, and in event level we don't expect any overlap, should we keep the original assertion back there?

If we want to test that the events are not overlapping in straxen, we should do it in straxen's Event plugin.

@dachengx dachengx merged commit a31fb2b into master Jul 6, 2023
10 checks passed
@dachengx dachengx deleted the abs_time_to_prev_next_interval_warning branch July 6, 2023 17:55
@WenzDaniel
Copy link
Collaborator

Hej guys thanks. The only place we are using this function at the moment is the deadtime calculation in cutax. And here I think nothing can overlap (I hope at least).

@FaroutYLq
Copy link
Collaborator

Hi @WenzDaniel are you sure? I didn't find anything in cutax using this function, but I do find only one in straxen here

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

4 participants