XENONnT / straxen

Streaming analysis for XENON
BSD 3-Clause "New" or "Revised" License
21 stars 33 forks source link

Use SOM peaklets classification by default #1471

Closed dachengx closed 1 week ago

dachengx commented 1 week ago

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

Depends on https://github.com/XENONnT/straxen/pull/1472

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

SIGNIFICANT CHANGE BREAKING CHANGE

Can you briefly describe how it works?

Use SOM classification (https://xe1t-wiki.lngs.infn.it/doku.php?id=xenon:xenonnt:lsanchez:som_summary_note) by default.

Following https://github.com/XENONnT/straxen/pull/1269 and https://github.com/XENONnT/straxen/pull/1300.

Can you give a minimal working example (or illustrate with a figure)?

Please include the following if applicable:

Notes on testing

All italic comments can be removed from this template.

sebvetter commented 1 week ago

Do we want the default plugins to be called pluginSOM? E.g. straxen.PeaksSOM, straxen.PeakBasicsSOM,

dachengx commented 1 week ago

Do we want the default plugins to be called pluginSOM? E.g. straxen.PeaksSOM, straxen.PeakBasicsSOM,

I do not like it, either. Any suggestions? :)

sebvetter commented 1 week ago

What is the problem with simply naming the class Peaks, or rather overwriting the old Peaks plugin with the contents of the PeaksSOM plugin? The old code will be preserved in older straxen versions. Will there still be some plugins after this PR that use the non-SOM versions?

dachengx commented 1 week ago

What is the problem with simply naming the class Peaks, or rather overwriting the old Peaks plugin with the contents of the PeaksSOM plugin? The old code will be preserved in older straxen versions. Will there still be some plugins after this PR that use the non-SOM versions?

E.g, PeakletClassificationSOM is inherited from PeakletClassification so we have to keep PeakletClassification.

But maybe rename PeakletClassification to something else?

coveralls commented 1 week ago

Coverage Status

coverage: 89.792% (+1.1%) from 88.685% when pulling 2bb7e68368baf4dc9b9b90a4380b64eedee6ebd3 on som_default into 28240ff1ba8f701794568af7be9481841c60fb15 on master.