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

Add warning #776

Merged
merged 8 commits into from
Dec 20, 2023
Merged

Add warning #776

merged 8 commits into from
Dec 20, 2023

Conversation

WenzDaniel
Copy link
Collaborator

@WenzDaniel WenzDaniel commented Nov 20, 2023

Add a small user warning if a plugin is deregistered as this is should be not a normal use case. Also converts list of plugins to deregistered into a set to avoid duplication. Small addition to: https://github.com/AxFoundation/strax/pull/775/files

Copy link

Coverage Status

coverage: 91.123% (-0.3%) from 91.472%
when pulling c0cecbb on add_warning
into 672dacb on master.

@coveralls
Copy link

coveralls commented Nov 20, 2023

Coverage Status

coverage: 91.352% (+0.002%) from 91.35%
when pulling 0383f06 on add_warning
into e102dbb on master.

@JelleAalbers
Copy link
Member

This is a good idea; I had this in #775 earlier but it generated quite a few messages when starting a cutax context. But maybe that's OK, or you want to merge this after fixing straxen/cutax first.

@WenzDaniel
Copy link
Collaborator Author

For straxen I did: XENONnT/straxen#1292
For cutax this is a bigger story because we are auto-registering cuts which we should not do in my opinion. But should be solved with: https://github.com/XENONnT/cutax/pull/317 I think.

JelleAalbers
JelleAalbers previously approved these changes Nov 29, 2023
Copy link
Member

@JelleAalbers JelleAalbers 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! I assume you want to wait until at least https://github.com/XENONnT/cutax/pull/317 is merged to avoid a swarm of warning messages, but from the strax side I don't see any problems.

(Well, there is a typo in the message, overlap dregister should probably be overlaps registered, but I guess it reinforces the message that you should not be seeing this message ;-)

The set doesn't change any behavior I think due to the check if old_plugin == currently_registered, but I think it make things clearer. (The purpose is to deregister plugins once for each datatype.)

@WenzDaniel
Copy link
Collaborator Author

Thanks @JelleAalbers, I fixed the typo. Regarding the set, I was under the impression that otherwise the error is raised multiple times, but I could be wrong now.

@WenzDaniel WenzDaniel merged commit 4ce63c2 into master Dec 20, 2023
11 checks passed
@WenzDaniel WenzDaniel deleted the add_warning branch December 20, 2023 12:33
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