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 timing of peaks when ordering in center_time #1208

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Jun 23, 2023

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What does the code in this PR do / what does it improve?

strax.touching_windows requires that the time of things and containers are all sorted.

When the time and endtime are defined based on center_time in some plugins like PeakShadow and PeakAmbience, the touching_windows will show an error, because the center_time is not sorted.

Reference: AxFoundation/strax#725

Can you briefly describe how it works?

First, order the peaks by center_time, after the features are calculated, recover the original order.

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?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.
  • On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

All italic comments can be removed from this template.

@coveralls
Copy link

Coverage Status

coverage: 93.53% (-0.003%) from 93.533% when pulling b2f133a on fix_shadow_time_order into 8dc4417 on master.

@dachengx dachengx requested a review from FaroutYLq June 23, 2023 22:12
Copy link
Contributor

@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. Just one quick sanity check: is it possible for two peaks to share same time? Seems no to me.

@FaroutYLq
Copy link
Contributor

FaroutYLq commented Jun 24, 2023

Will it be possible that a merged_s2s has same time as an S1 fully contained in it?

@dachengx
Copy link
Collaborator Author

dachengx commented Jun 24, 2023

Looks good to me. Just one quick sanity check: is it possible for two peaks to share same time? Seems no to me.

We have no functions to prevent that.

Similar case to center_time. But the mergesort algorithm of argsort make itself a stable sorting so that even though the center_time of two peaks can be the same, they will still be reordered correctly in the final result, because the argsort of two same center_time is still in the order of indices when sorting is stable.

@FaroutYLq
Copy link
Contributor

Looks good to me. Just one quick sanity check: is it possible for two peaks to share same time? Seems no to me.

We have no functions to prevent that.

Similar case to center_time. But the mergesort algorithm of argsort is stable sorting so that even though the center_time of two peaks can be the same, they will still be reordered correctly in the final result, because the argsort of two same center_time is still in the order of indices when it is stable.

Then I have no concern. Please feel feee to merge it.

@dachengx dachengx merged commit a952805 into master Jun 24, 2023
@dachengx dachengx deleted the fix_shadow_time_order branch June 24, 2023 06:47
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.

3 participants