Update som classifcation #1300

WenzDaniel · 2023-11-30T10:19:24Z

This PR will change the lineage for > peaklets!

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

In this PR we change the SOM plugin such that it only returns a single data-type. It is based on the changes made in #1299. Further, we propagate the SOM and straxen classification, including the SOM subclasses, up to event basics for a better analysis.

I test processed a short run 025423 with the normal offline and SOM context.

…erited

…ation

JYangQi00 · 2023-12-15T18:22:07Z

straxen/contexts.py

+            straxen.PeakBasicsSOM,
+            straxen.EventBasicsSOM,
+        )
+    )



Just for clarification, the PeaksSOM, PeakBasicsSOM, and EventBasicsSOM is so that we do not interfere with the lineage of the non-SOM plugins?

I had a few more thoughts about it during the weekend. This is the idea yes. However, this is only true for not using SOM at all. If we want to use SOM as a cut we also have to use the SOM plugins to do it properly.

The lineages of the two will always be separate since the peaklet clasification will have a different liniage as that propagates all the way down the processing pipeline, I thought the main Idea was just to propagate the SOM clusters down the chain no?

Well yes and no, when I made this PR I made different assumptions. Right now one cannot separate the two.

JYangQi00 · 2023-12-15T18:30:57Z

straxen/plugins/events/event_basics_som.py

+        self.peak_properties = tuple(self.peak_properties)
+
+    def compute(self, events, peaks):
+        result = super().compute(events, peaks)


I presume that the super().compute() will copy the peak info into EventBasics. But just for clarification, the reason we don't need to use depends_on = (..., peaks_som) or something like that is because peaks itself is being provided by the PeaksSOM plugin which is registered in the context?

Yes, but the reason is PeakBasicsSOM provides a different version of peak_basics which EventBasics(SOM) depends on.

JYangQi00 · 2023-12-15T18:32:38Z

straxen/plugins/merged_s2s/merged_s2s.py

@@ -16,7 +16,7 @@ class MergedS2s(strax.OverlapWindowPlugin):
    """Merge together peaklets if peak finding favours that they would form a single peak
    instead."""

-    __version__ = "1.0.2"
+    __version__ = "1.1.0"


Presumably changes in this file are all from PR #1299?

Yes, they are. Sorry that it is a bit messy, but I thought you guys would look first at #1299

JYangQi00 · 2023-12-15T18:35:02Z

straxen/plugins/peaklets/peaklet_classification.py

@@ -12,7 +12,7 @@
 class PeakletClassification(strax.Plugin):
    """Classify peaklets as unknown, S1, or S2."""

-    __version__ = "3.0.3"
+    __version__ = "3.0.4"


I have no problems with the change here, but out of curiosity, why did we return peaklet_classification as a dict before? Also, @FaroutYLq , will this be an issue for reprocessing if the version changes but the output is functionally the same?

Thanks for the headup @JYangQi00 , very important catch! I would try to avoid touching the standard peaklet_classification in this SOM PR. It doesn't look to me that the performance of peaklet_classification is meaningfully changed, and if it is just about style we should also have a different PR for this. I would try to avoid bumping version which changes lineage so we have to reprocess again the standard pipeline.

The change is coming from #1299 and indeed the version bump is purely optional. Since I do not modify the outcome of the plugin we can also merge without version bump if you guys prefer. If you agree I change #1299.

JYangQi00 · 2023-12-15T18:50:26Z

straxen/plugins/peaks/peak_basics.py

@@ -15,40 +15,10 @@ class PeakBasics(strax.Plugin):

    """

-    __version__ = "0.1.4"
+    __version__ = "0.1.5"


Presumably this is also just from PR #1299 ?

Yes, same as above purely optional.

FaroutYLq

I agree with concern by @WenzDaniel and suggest we don't touch the regular peaklet_classification in this PR. The rest look good to me, but I have to admit I didn't run tests locally.
Another general comments: I think for this level of complexity and significance in update we should add some tests. I would let strax* maintainers to comment more here.

FaroutYLq · 2023-12-15T18:49:33Z

straxen/plugins/peaklets/peaklet_classification.py

@@ -12,7 +12,7 @@
 class PeakletClassification(strax.Plugin):
    """Classify peaklets as unknown, S1, or S2."""

-    __version__ = "3.0.3"
+    __version__ = "3.0.4"


Thanks for the headup @JYangQi00 , very important catch! I would try to avoid touching the standard peaklet_classification in this SOM PR. It doesn't look to me that the performance of peaklet_classification is meaningfully changed, and if it is just about style we should also have a different PR for this. I would try to avoid bumping version which changes lineage so we have to reprocess again the standard pipeline.

straxen/plugins/peaklets/peaklet_classification.py

JYangQi00 · 2023-12-15T18:53:52Z

straxen/plugins/peaks/peaks_som.py

+    def compute(self, peaklets, merged_s2s):
+        result = super().compute(peaklets, merged_s2s)
+
+        # For merged S2s SOM and straxen type are undefined:


Perhaps this is a really basic thing that I'm missing, but why are types undefined for merged S2s?

Because SOM classifies only based on peaklets. Merged S2s is only called on S2s (type==2) and we assume that these merged peaks stay S2s (type==2). However, for each S2 there are multiple SOM sub-types and it is currently not defined which sub-type should be inherited to the merged S2s after merging. We could do something like the sub-type of the peak with the largest area, but not sure if this make sense.
If one uses straxen types as classification for peaks then of course type == straxen_type and the straxen_type of MergedS2s would be defined again, however in that case the information is trivial to retrieve. So I preferred to not add an extra if case and make the code more complicated.

JYangQi00

Hi Daniel, thanks for the improvements! Everything looks fine to me I am just confused about a few things that I left in the comments. If you think they are fine then I will approve.

…raxen into make_peaklets_dtype_flexiable

WenzDaniel · 2023-12-18T08:20:35Z

Hmm @FaroutYLq which type of test do you have in mind? I can try to add some in #1299 because there we make actually all technical changes.

…ation

… only

WenzDaniel

Hey guys I think I made all needed modifications. I also added some comments to the code for guidance. I conducted the following test to see if everything works as intended:

Process run 025423 up to event basics using the current master using the default offline context. Switch to this branch and see if I can still load peaklet_classifcation, merged_s2s, peak_basics and event_basics with the default nT offline context.
Processed for the same run the three new SOM data-types and checked if I can load them with meaningful values too.
I repeated the test of 1. with run 053494 using the XENONnT online context.

If you guys do not have any objections I think we can merge.

WenzDaniel · 2023-12-20T12:05:45Z

straxen/contexts.py

@@ -19,12 +19,15 @@
        straxen.PulseProcessing,
        straxen.Peaklets,
        straxen.PeakletClassification,
+        straxen.PeakletSOMClass,


These three plugins registered to the default nT context are additional plugins. They will allow us to keep the already processed data as is and provide the additional SOM fields at peak and event level as additional data-types. Thus, after processing on midway we have to use OSG to process peak_som_classification and event_som_classification.

WenzDaniel · 2023-12-20T12:09:04Z

straxen/contexts.py

+            straxen.PeakletClassificationSOM,
+            straxen.PeaksSOM,
+            straxen.PeakBasicsSOM,
+            straxen.EventBasicsSOM,


These are new plugins which extend the already existing classical versions. With this context we will have a new lineage for everything above peaklets. The same data-type names as for the classical case are used (peaklet_classification, peaks, peak_basics, event_basics), such that we can use the standard processing procedure.

PeakletClassificationSOM computes first the classical classification before adding the SOM information.

PeaksSOM propagates the information to Peaks such that we can propagate it to peak basics in PeakBasicsSOM.

EventBasicsSOM is the same as the classical EventBasics, but also extended by the additional SOM fields.

WenzDaniel · 2023-12-20T12:11:07Z

straxen/plugins/events/event_basics_som.py

+
+
+@export
+class EventBasicsSOM(EventBasics):


This plugin is inherited from the classic event-basics and only adds in addition the new SOM fields. It will also provide event_basics and is used in the SOM context.

WenzDaniel · 2023-12-20T12:11:41Z

straxen/plugins/events/event_basics_som.py

+
+
+@export
+class EventSOMClassification(strax.Plugin):


This plugin is the extra plugin for the default context case in which we provide in addition event_som_clasification. For peaks which do not provide any SOM information like S2s -1 is set as default value.

WenzDaniel · 2023-12-20T12:14:33Z

straxen/plugins/peaklets/peaklet_classification.py

-            channel=-1,
-            length=peaklets["length"],
-        )
+        peaklets_classification = np.zeros(len(peaklets), dtype=self.dtype)


Here I change the plugin return from a dict into an array. There is no impact whatsoever for our processing. Only more convenient to work with when inhering.

WenzDaniel · 2023-12-20T12:17:41Z

straxen/plugins/peaklets/peaklet_classification_som.py

+
+
+@export
+class PeakletSOMClass(PeakletClassificationSOM):


This plugin is used in the default straxen context as the provided data-type needs to be different from peaklet_classification. The rest is inherited from above.

WenzDaniel · 2023-12-20T12:18:01Z

straxen/plugins/peaks/peak_basics.py

@@ -66,6 +36,45 @@ class PeakBasics(strax.Plugin):
        ),
    )

+    def infer_dtype(self):


Only moving the dtype in here so it can be better inherited.

WenzDaniel · 2023-12-20T12:21:02Z

straxen/plugins/peaks/peak_basics_som.py

+
+
+@export
+class PeakBasicsSOM(PeakBasics):


This plugin is used in the SOM context and replaces peak_basics with a new version including the SOM parameters.

WenzDaniel · 2023-12-20T12:21:45Z

straxen/plugins/peaks/peaks_som.py

+
+
+@export
+class PeaksSOM(Peaks):


This plugin is used in the SOM context to propagate the SOM parameters from peaklet_classification to peaks.

WenzDaniel · 2023-12-20T12:26:00Z

straxen/plugins/peaks/peaks_som.py

+
+
+@export
+class PeaksSOMClassification(strax.Plugin):


This is again a stand alone plugin to propagate the SOM settings to peaks for the default nT context using the default classification.

coveralls · 2023-12-20T18:03:31Z

coverage: 92.417% (+0.8%) from 91.571%
when pulling 6e5d7b5 on update_som_classifcation
into c7916fc on master.

JYangQi00 · 2023-12-21T00:22:00Z

I checked that the That peaklet_classification (SOM context) has the same straxen_type as the type of peaklet_classification (nT online context), and the same som fields as peaklet_classification_som (also in nT context). I did similar things as well for the peak and event basics as well, but only looking at the S1 SOM information (as intended by this PR). Everything looks good on my end and I think we are good to merge!

WenzDaniel added 16 commits November 29, 2023 07:56

Add proper dtype inheritance

31c6ea8

Let Peaks decide propagated peaklet dtypes.

83f05ff

Small fix

163ac29

Change dict return into array for simpler inheritance

fc7d66a

Remove duplicated code, use single provided datatype

5d7da77

Add name for copy function to ensure correct caching if plugin is inh…

eabc532

…erited

Add inherited plugin to propagate SOM properties

b11639f

Move dtype definition for simpler plugin inheritance

881e573

Add SOM peak basics to propgate som properties

44a3795

Add new inits

cd210f1

Add SOM event basics plugin

c3cc8cd

Add plugin to init

03f2be5

Add plugins to context

35beda9

Change undefined types to -1

ac8b90a

Add again if statement

292c910

Merge branch 'make_peaklets_dtype_flexiable' into update_som_classifc…

18ab5b6

…ation

WenzDaniel mentioned this pull request Dec 13, 2023

Add hot fix for copy_to_buffer AxFoundation/strax#785

Merged

FaroutYLq requested review from LuisSanchez25 and JYangQi00 December 14, 2023 16:15

JYangQi00 reviewed Dec 15, 2023

View reviewed changes

FaroutYLq requested changes Dec 15, 2023

View reviewed changes

JYangQi00 reviewed Dec 15, 2023

View reviewed changes

WenzDaniel and others added 3 commits December 18, 2023 08:06

Merge branch 'master' into make_peaklets_dtype_flexiable

1862e9d

Make line breaks in comments

bcf4080

Merge branch 'make_peaklets_dtype_flexiable' of github.com:XENONnT/st…

3c7b733

…raxen into make_peaklets_dtype_flexiable

WenzDaniel added 12 commits December 19, 2023 01:42

Do not bump version to keep lineage

8afd915

Merge branch 'master' into make_peaklets_dtype_flexiable

d3c9723

Add back in option to preserve lineage....

284206e

Merge branch 'make_peaklets_dtype_flexiable' into update_som_classifc…

ce7c84f

…ation

Added option for SOM/straxen, made plugin to propagte SOM for straxen…

09b0442

… only

Add plugin to propagate SOM to peaks

fe81e8c

Add plugin to propagate SOM to events

2db4f04

Update context

803660a

Merge branch 'master' into update_som_classifcation

cbffd4b

Revert not needed changes

f7a3a65

Fix some typos

f3ad7d2

Fix typo

ace093e

WenzDaniel commented Dec 20, 2023

View reviewed changes

WenzDaniel and others added 2 commits December 20, 2023 17:53

Merge branch 'master' into update_som_classifcation

39a5791

Move SOM plugin registration

15e9e31

Merge branch 'master' into update_som_classifcation

6e5d7b5

FaroutYLq approved these changes Dec 20, 2023

View reviewed changes

dachengx merged commit f3e7726 into master Dec 21, 2023
9 checks passed

dachengx deleted the update_som_classifcation branch December 21, 2023 11:17

dachengx mentioned this pull request Nov 15, 2024

Use SOM peaklets classification by default #1471

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update som classifcation #1300

Update som classifcation #1300

WenzDaniel commented Nov 30, 2023

JYangQi00 Dec 15, 2023

WenzDaniel Dec 18, 2023

LuisSanchez25 Dec 18, 2023

WenzDaniel Dec 19, 2023

JYangQi00 Dec 15, 2023

WenzDaniel Dec 18, 2023

JYangQi00 Dec 15, 2023

WenzDaniel Dec 18, 2023

JYangQi00 Dec 15, 2023

FaroutYLq Dec 15, 2023

WenzDaniel Dec 18, 2023

JYangQi00 Dec 15, 2023

WenzDaniel Dec 18, 2023

FaroutYLq left a comment

FaroutYLq Dec 15, 2023

JYangQi00 Dec 15, 2023

WenzDaniel Dec 18, 2023

JYangQi00 left a comment

WenzDaniel commented Dec 18, 2023

WenzDaniel left a comment

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

WenzDaniel Dec 20, 2023

coveralls commented Dec 20, 2023 •

edited

Loading

JYangQi00 commented Dec 21, 2023

Update som classifcation #1300

Update som classifcation #1300

Conversation

WenzDaniel commented Nov 30, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FaroutYLq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JYangQi00 left a comment

Choose a reason for hiding this comment

WenzDaniel commented Dec 18, 2023

WenzDaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2023 • edited Loading

JYangQi00 commented Dec 21, 2023

coveralls commented Dec 20, 2023 •

edited

Loading