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

Loopplugin touching windows + plugin documentation #424

Merged

Conversation

JoranAngevaare
Copy link
Member

@JoranAngevaare JoranAngevaare commented Apr 10, 2021

What is the problem / what does the code in this PR do
As explained in #221, having a touching windows option might be nice to add in the LoopPlugins. This PR adds that.

Experimental
Note: as Daniel correctly pointed out (#424 (review)), this may lead to ambiguity if for example one peak is touching the window of two events, this would mean that if one is looping over events, that one peak is included twice. Depending on the application, this may or may not be an issue.

For now, we added a warning to alert the user of this (potential) issue.

Can you briefly describe how it works?
It's only a time-selection that allows to slightly extend the time range.

Can you give a minimal working example (or illustrate with a figure)?
Please see the tests for an example.

Documentation
Has been updated, additionally, I wrote out some examples that might be useful and work without any straxen installation.

afbeelding

@JoranAngevaare JoranAngevaare added documentation Something should be addressed in the docs enhancement New feature or request labels Apr 10, 2021
@JoranAngevaare JoranAngevaare changed the title Loopplugin touching windows Loopplugin touching windows + plugin documentation Apr 10, 2021
@JoranAngevaare JoranAngevaare added the Testing Works on testing code label Apr 11, 2021
Copy link
Collaborator

@WenzDaniel WenzDaniel left a comment

Choose a reason for hiding this comment

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

Ah here we have to be careful. At least we should discuss it. For touching windows you can become ambiguities and use the same data twice which is not desirable. E.g. if you have a peak which touches two adjacent events it will be associated with both. See as an example: https://github.com/XENONnT/straxen/blob/a49c51ac1fda5be8e35f1ff19b4637d7a89700da/straxen/plugins/veto_events.py#L173-L181 in which I had to deal with such cases.

While I think such a plugin could be in principle useful, I am not sure if it will be easy to find a general rule about how to solve these ambiguites.

@JoranAngevaare
Copy link
Member Author

@WenzDaniel , thanks, valid point. While I agree that this a potential issue, it is not per se a bad thing, people should be careful with what they do with that data indeed. If you want to have e.g. a more meta-number like how many peaks are at least partially overlapping with an event, having peaks taken into account twice might not be a huge problem.

I don't have some concrete examples where this would be used, I think there is no a-priori problem if we allow this to be supported.

How about the following:

  • I add a comment in the plugin;
  • I for now raise a warning in the plugin when this option is ever used (it can be disabled then if needed)

@WenzDaniel
Copy link
Collaborator

Yes, I agree we should add both a comment and a warning.

@JoranAngevaare
Copy link
Member Author

@WenzDaniel , thanks again done, also added a reference to this PR as well as updated the description of this PR.

Copy link
Collaborator

@WenzDaniel WenzDaniel left a comment

Choose a reason for hiding this comment

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

Thanks

@JoranAngevaare
Copy link
Member Author

@WenzDaniel thanks for the review and the good comment 😄

@JoranAngevaare JoranAngevaare merged commit 9e68d20 into AxFoundation:master Apr 12, 2021
@JoranAngevaare JoranAngevaare deleted the loop_touching_windows branch April 12, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Something should be addressed in the docs enhancement New feature or request Testing Works on testing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants