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

Allow saving peaklets in main and alt peaks #1230

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shenyangshi
Copy link
Contributor

Allow saving peaklets in main and alt S1 and S2
image

@@ -77,9 +77,14 @@
from .peaks_he import *
from . import peaks_he

from . import peaklets_main_alt
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
F401 '.peaklets_main_alt' imported but unused

@@ -77,9 +77,14 @@
from .peaks_he import *
from . import peaks_he

from . import peaklets_main_alt
from .peaklets_main_alt import *
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
F401 '.peaklets_main_alt.*' imported but unused

@@ -77,9 +77,14 @@
from .peaks_he import *
from . import peaks_he

from . import peaklets_main_alt
from .peaklets_main_alt import *
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS347 Found vague import that may cause confusion: *

@@ -77,9 +77,14 @@
from .peaks_he import *
from . import peaks_he

from . import peaklets_main_alt
from .peaklets_main_alt import *
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS458 Found imports collision: peaklets_main_alt

# Misc
from . import afterpulses
from .afterpulses import *

from . import led_cal
from .led_cal import *


Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
W391 blank line at end of file

self.fake_mask(events, f'{tag}_time',
f'{tag}_endtime')) # touch the peaklets
if len(result) > 0:
result = np.concatenate([np.arange(result[i][0], result[i][1], dtype=int) for i in
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS221 Found line with high Jones Complexity: 22 > 14

self.fake_mask(events, f'{tag}_time',
f'{tag}_endtime')) # touch the peaklets
if len(result) > 0:
result = np.concatenate([np.arange(result[i][0], result[i][1], dtype=int) for i in
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS361 Found an inconsistently structured comprehension

f'{tag}_endtime')) # touch the peaklets
if len(result) > 0:
result = np.concatenate([np.arange(result[i][0], result[i][1], dtype=int) for i in
range(len(result))]) # find the index of the touched peaklets
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (103 > 100 characters)

result = np.concatenate([np.arange(result[i][0], result[i][1], dtype=int) for i in
range(len(result))]) # find the index of the touched peaklets
peaklets_main_alt_id.append(np.array(result))
# peaklets_main_alt = peaklets[np.array(peaklets_main_alt_id)]
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

peaklets_main_alt_id.append(np.array(result))
# peaklets_main_alt = peaklets[np.array(peaklets_main_alt_id)]
peaklets_main_alt = peaklets[np.sort(np.concatenate(peaklets_main_alt_id))] # sort by time
peaklets_main_alt = peaklets_main_alt[np.argsort(peaklets_main_alt['time'])] # sort again just in case
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (111 > 100 characters)

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.

Hej Shenyang, thanks but I would like to ask some changes. I think the code can be much simpler.

Comment on lines +34 to +35
def setup(self):
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

def fake_mask(self, df, start_field='time', end_field='end_time', pad=10e3):
"""
Create a fake mask to touch the peaklets
:param df: events
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call it events and not df for the sake of clean code and readability. If you call it events I know directly what the object is suppose to represent. Like it is know I will think that I have to provide a dataframe here which is most likely wrong as plugins handle data in from of structure arrays.

In addition I think the entire function is not needed. See my comment below.

Comment on lines +64 to +73
for tag in ['s1', 'alt_s1', 's2', 'alt_s2']:
result = strax.touching_windows(peaklets,
self.fake_mask(events, f'{tag}_time',
f'{tag}_endtime')) # touch the peaklets
if len(result) > 0:
result = np.concatenate([np.arange(result[i][0], result[i][1], dtype=int) for i in
range(len(result))]) # find the index of the touched peaklets
peaklets_events_id.append(np.array(result))
peaklets_events = peaklets[np.sort(np.concatenate(peaklets_events_id))] # sort by time
peaklets_events = peaklets_events[np.argsort(peaklets_events['time'])] # sort again just in case
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be done easier. We provide an index field for peaks in a given event is not it? You can simply do:

peaks_indicies_in_event = strax.touching_windows(peaks, events)

for indicies, event in zip(peaks_indicies_in_event , event):
    start_index, end_index = indicies
    peaks_in_event = peaks[start_index:end_index]
    s1_in_event = peaks_in_event[event['s1_index']] 

You can even numbafy this for loop if necessary. You know that you can have only n_events S1/S2 etc. So you fill first the corresponding buffer in the numbafied for-loop, drop all null results and concetante and sort afterwards,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, I made a mistake to make this work via the index you need to use fully_contained_in or split_by_containment instead of touching_windows. See:

split_peaks = strax.split_by_containment(peaks, events)

and
https://github.com/AxFoundation/strax/blob/9b508f7f8d441bf1fe441695115d292c59ce631a/strax/processing/general.py#L224

@WenzDaniel
Copy link
Collaborator

Long time no new news, converting to a draft.

@WenzDaniel WenzDaniel marked this pull request as draft November 22, 2023 07:14
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.

2 participants