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

Refactor nv plugins #1228

Merged
merged 22 commits into from
Aug 28, 2024
Merged

Refactor nv plugins #1228

merged 22 commits into from
Aug 28, 2024

Conversation

WenzDaniel
Copy link
Collaborator

@WenzDaniel WenzDaniel commented Aug 1, 2023

Refactor NV plugins:

  • Removes computation of not needed parameters.
  • Adds new plugin which computes the summed waveform for NV events.
  • Some minor readability changes

Requires AxFoundation/strax#744 to be merged. After meging this PR we need to change the data types which are produced and shipped from the EBs. In addition, to hitlets, events_nv and event_positions_nv, event_wavefrom_nv must be made and shipped. The total amount of compressed data will be ~30 % smaller than before.

@WenzDaniel WenzDaniel marked this pull request as draft August 1, 2023 07:49
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

pep8

straxen/plugins/events_nv/event_waveform_nv.py|94 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|44 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|63 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|64 col 1| D102 Missing docstring in public method
straxen/plugins/events_nv/events_nv.py|71 col 9| WPS221 Found line with high Jones Complexity: 15 > 14
straxen/plugins/events_nv/events_nv.py|72 col 9| WPS362 Found assignment to a subscript slice
straxen/plugins/events_nv/events_nv.py|74 col 1| D102 Missing docstring in public method
straxen/plugins/events_nv/events_nv.py|80 col 84| W291 trailing whitespace
straxen/plugins/events_nv/events_nv.py|81 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|93 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|94 col 1| W293 blank line contains whitespace
straxen/plugins/events_nv/events_nv.py|101 col 21| E124 closing bracket does not match visual indentation
straxen/plugins/events_nv/events_nv.py|255 col 9| WPS221 Found line with high Jones Complexity: 15 > 14

straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
straxen/plugins/events_nv/event_waveform_nv.py Outdated Show resolved Hide resolved
@WenzDaniel WenzDaniel marked this pull request as ready for review August 1, 2023 12:12
@WenzDaniel
Copy link
Collaborator Author

Long time no news converting into draft.

@WenzDaniel WenzDaniel marked this pull request as draft November 22, 2023 07:15
@WenzDaniel WenzDaniel marked this pull request as ready for review August 27, 2024 13:03
@WenzDaniel
Copy link
Collaborator Author

Tests can only pass once the strax PR is merged

@coveralls
Copy link

coveralls commented Aug 27, 2024

Coverage Status

Changes unknown
when pulling 327fa79 on refactor_nv_plugins
into ** on master**.

Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

Thank you @WenzDaniel . Technically it looks good.

@dachengx dachengx merged commit 6ad09a1 into master Aug 28, 2024
8 checks passed
@dachengx dachengx deleted the refactor_nv_plugins branch August 28, 2024 13:20
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