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

Merge making distinct_channels and event_area_per_channel into single multi-output plugin #617

Open
JoranAngevaare opened this issue Aug 1, 2021 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@JoranAngevaare
Copy link
Contributor

Their code / setup is basically the same. Might as well merge it into a single plugin to reduce processing complexity.

One potential caveat is that the save_when attribute is set for an entire plugin, wheres we are saving distinct_channel we are not doing so for event_area_per_channel (by default) as that is a rather big datatype (in contrast to distinct_channel). If we implement AxFoundation/strax#504 this argument would render moot.

@JoranAngevaare JoranAngevaare added enhancement New feature or request good first issue Good for newcomers labels Aug 1, 2021
@terliuk
Copy link
Contributor

terliuk commented Aug 2, 2021

I think it makes sense to make it even more generic and add waveform information for the peaks, such as waveform data (eventually, with top+bottom waveforms when we have them). I think there are only 4 peaks of interest in each events, so that data should be really small, but can help to make extra studies or algorithms applied at event level. What do you think about it?

@JoranAngevaare
Copy link
Contributor Author

JoranAngevaare commented Aug 2, 2021

Not a bad idea. There are some things to take into account for such a scheme:

  • storage -> Will take a bit more if we have a very high level plugin that might change therefore change often that has some considerable size (might need to verify what "considerable" is in this context).
  • loading main/alt S1/S2 properties -> In principle, loading these from Peaks should be fine/fast enough (also in plugins). It's not like it's impossible to load the data on event level from peaks, and we are usually shorter on storage than on CPU. On the other hand if the main+alt S1s/S2s peaks incl. sum wfs are stored separately, this will reduce RAM and loading time if you do need to load/compute these kind of things at event level.

Taken these to things into account, I could argue either way and have no strong preference for either. Given that you say it would benefit your work, I would say it's good to implement it.

Perhaps it's worth asking some other experts like @ershockley or @WenzDaniel for their opinions.

@WenzDaniel
Copy link
Collaborator

I think processing on event level is usually quite quick. Maybe to have the best of both worlds, I agree with your suggestions and 1. make it a separate plugin 2. make the plugin save when target. In that case analysts can decided on their own if they need the additional information or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants