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

Make peaklets dtype flexiable #1299

Merged
merged 11 commits into from Dec 19, 2023
Merged

Conversation

WenzDaniel
Copy link
Collaborator

Side note this PR will change the lineage of everything > peaklets!

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

Simple PR which makes the dtype propagation for peaklets to peaks more flexible. In the current implementation the dtype of merged S2s and peaks is fixed to the peaklets_dtype which is fixed to the peaks_dtype defined in strax. Already in the past this lead to an issue when we added new optional fields to peaks. For example when adding the hit timing https://github.com/AxFoundation/strax/blame/21cc96e011b0e4099138979791f34e8b1addedb7/strax/dtypes.py#L211 or top waveform https://github.com/AxFoundation/strax/blame/21cc96e011b0e4099138979791f34e8b1addedb7/strax/dtypes.py#L222. Especially, for the latter we added an if statement into merged_S2s to also include the top_data field when doing so on the peaklet level.
Also for the SOM peaklet_classifcation this lead to issues because no additional fields could be added to peaklet_classifcation without also changing the peaklet data-type.

This is in general a not very strax-like behavior in which we know for each plugin its dependencies and the data-types of these dependencies.

Can you briefly describe how it works?

To circumvent this issue and to make the peaklet and peaklet_classification data-type more flexiable, I compute now the merged_S2s dtype based on the peaklets and peaklet_classifcation dtypes of the currently registered plugins. This also allows us to remove the additional if-statement in merged_s2s needed for the top waveform. In this way analysts can add as many fields to their own peaklet and classification plugins as they like. To be still able to control which parameters are propagated from peaklets to peaks (and higher up) I added a copy statement at the end of peaks which only copies all fields from peaklets which are provided by the Peaks plugin. The additional computation time is negligible.

Although the changes do not change the data provided by the plugins, I bumped the code version.

@WenzDaniel
Copy link
Collaborator Author

Edit: We still need the if statement in merged_s2s because the functions which build the merged S2s require the field...

@FaroutYLq
Copy link
Contributor

I am happy to review this one. Isn't this PR more about peaks rather than peaklets? If it change peaklets lineage, the time for v14 reprocessing is gonna be way longer.

@FaroutYLq FaroutYLq self-requested a review December 12, 2023 18:22
@WenzDaniel
Copy link
Collaborator Author

@FaroutYLq thanks, yes it only changes the lineage of everything above peaklets. I also have an idea how to fix the remaining test failure. This requires a change in strax though. I hope I can add it today.

@WenzDaniel
Copy link
Collaborator Author

WenzDaniel commented Dec 13, 2023

@FaroutYLq this PR requires: AxFoundation/strax#785 once it is merged it should work. If you have time it would be nice if you could review the other one as well.

@coveralls
Copy link

coveralls commented Dec 18, 2023

Coverage Status

coverage: 92.815% (-0.007%) from 92.822%
when pulling 3c7b733 on make_peaklets_dtype_flexiable
into f23319d on master.

@WenzDaniel
Copy link
Collaborator Author

I changed back the plugin version and added back in redundant option to preserve the lineage.

@WenzDaniel WenzDaniel mentioned this pull request Dec 19, 2023
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.

I think that it is better to keep the version number since the results do not change.

sum_waveform_top_array = straxen.URLConfig(
default=True, type=bool, help="Digitize the sum waveform of the top array separately"
)

n_top_pmts = straxen.URLConfig(type=int, help="Number of top TPC array PMTs")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these lines are moved several lines below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah because I added again back in sum_waveform_top_array must have added it at the wrong position. Otherwise the lineage would change. Even though the option is not needed anymore here.

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 approve the PR if moved back. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very picky, I like it ;) :D I will do just a sec.

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.

Thanks @WenzDaniel. It looks good to me now.

@dachengx
Copy link
Collaborator

I have checked that the lineage of peaks does not change.

@WenzDaniel WenzDaniel merged commit c532353 into master Dec 19, 2023
9 checks passed
@WenzDaniel WenzDaniel deleted the make_peaklets_dtype_flexiable branch December 19, 2023 16:38
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

4 participants