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

Fix nveto processing #491

Merged
merged 5 commits into from Jul 28, 2021
Merged

Fix nveto processing #491

merged 5 commits into from Jul 28, 2021

Conversation

WenzDaniel
Copy link
Collaborator

What is the problem / what does the code in this PR do
During my investigation for straxen #596 I noticed that in rare cases hitlet splitting produces zero length rows if new peaks are not sorted by time. Given the nature of records and hitlets compared to peaks this can happen from time to time. In this PR we add an explicit sorting and raise condition if zero-length peaks are supposed to be returned after splitting as this is not a desired behavior.

Unfortunately, I do not belief that this change will solve #596.

@JoranAngevaare
Copy link
Member

Thanks Daniel,
Would it for explicitness not be better to do this step in the plugin computation in straxen? I think that might be clearer than making this additional step in the peak splitting routine?

@WenzDaniel
Copy link
Collaborator Author

This would not work as we split here recursively. Hence one has to sort before L103-106.

@JoranAngevaare
Copy link
Member

Sorry, perhaps I am a bit slow after my vacation but why would, after splitting, the time-ordering not be preserved?

@WenzDaniel
Copy link
Collaborator Author

WenzDaniel commented Jul 28, 2021

For peaks it is yes but not for hitlets as you have them for each channel.
Lets assume we split a hitlet in channel 1 into two:

ch1.: |-----|-----|
ch2.:      |----|

If you do not sort again the order would be ch_1_1 ch_1_2 ch_2 while the correct order is ch_1_1, ch_2, ch_1_2.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Thanks Daniel

@WenzDaniel WenzDaniel merged commit b3fa4fd into master Jul 28, 2021
@WenzDaniel WenzDaniel deleted the fix_nveto_processing branch July 28, 2021 10:54
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

2 participants