Implement Pyrrha functionalities in Irene by Ian0sborne · Pull Request #953 · next-exp/IC

Ian0sborne · 2026-03-05T16:34:47Z

This PR addresses the data processing issues cause by the SiPM thresholds in Irene. It retains standard Irene functionalities but also implements the new Pyrrha selection methods for SiPMs, which relies on a spatial selection rather than a charge threshold.

The user is free to decide how they would rather process the raw waveforms (using the old Irene or Pyrrha), as the config file allows for either method to be selected.

Done for backwards compatibility, this function now mirrors that of `calibrate_sipms()` before thresholding was removed.

This is the main controlling function, that decides which type of cut will be applied.

This included the adjustment of the cuts, applying `select_wfs_above_time_integrated_thr()` and the zeroing integration cut before the waveform rebinning.

This is to avoid possible situations where `calibrate_sipms()` is used outwith irene. `thr` is now a keyword argument to ensure that if a threshold is applied it is done so with intent.

@Ian0sborne

Initially this was done over the entire waveforms due to the reordering of the cuts when applied in `build_sipm_responses()`. As suggested by @Ian0sborne, it would be wise to move these cuts out of `compute_and_write_pmaps()`

…g_function`

…ection`) into `pyrrha_sipm_selection`

…iPMs and their waveforms

…ods for pyrrha

…SiPMSelectionMethod class

…for incorrect input

Ian0sborne · 2026-05-07T15:33:02Z

+    if thr is None:
+        return cwfs
+    else:
+        # if you apply a threshold here, apply as usual
+        thr  = to_col_vector(np.full(sipm_wfs.shape[0], thr))
+        return np.where(cwfs > thr, cwfs, 0)


Even though this is not used in the code, this is needed for 4 tests in calib_sensor_functions_test.py.

Add another function that does the thresholds suppression, redo those tests doing the threshold suppression outside calibrate_sipms, and finally remove the else branch here

The function charge_threshold_method() in reco/wfm_functions.py does the zero suppression step. Should I make a function that does just that? If I do this, do I remove the test that rely on zero suppression from calib_sensor_functions_test.py and implement them specifically for this new function?

…t to 0

…ate for it

…eshold outside of `charge_threshold_method()`

…_wfs_below_threshold()` for zeroing waveform entries

gonzaponte

I will take a deeper look, but here is a first go.

gonzaponte · 2026-06-04T16:18:11Z



 def waveform_integrator(limits):
    def integrate_wfs(wfs):
        return cf.spaced_integrals(wfs, limits)[:, ::2]
    return integrate_wfs


 # Compound components
 def compute_and_write_pmaps(detector_db, run_number, pmt_samp_wid, sipm_samp_wid,
                  s1_lmax, s1_lmin, s1_rebin_stride, s1_stride, s1_tmax, s1_tmin,
                  s2_lmax, s2_lmin, s2_rebin_stride, s2_stride, s2_tmax, s2_tmin, thr_sipm_s2,
-                  h5out, sipm_rwf_to_cal=None):
+                  h5out, apply_cut, sipm_rwf_to_cal=None):


I think we need a better name for apply_cut. Think of something along the lines of "sipm selection algorithm"

I agree, I'll do sipm_selection_algo, but I will change it last so there's no back and forth

gonzaponte · 2026-06-04T16:23:07Z

+    if apply_cut is not None:
+        # apply cut before slicing and rebinning
+        (sipm_ids,
+         sipm_wfs)   = apply_cut(sipm_wfs)
+    else:
+        # give all sipm ids as index if no cut is applied
+        sipm_ids     = np.arange(sipm_wfs.shape[0])
+


What does apply_cut = None represent? I thought there were only 2 options: threshold or pyrrha

I think this is a leftover from when we were first making the implementation, it doesn't make sense anymore I agree

However in the functions prior to this one, apply_cut is set as None in the parameters, see:

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR103

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR130

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR154

Should we leave this as is?

Okay I removed the else clause and some tests for the peak_functions.py use to the None case for the apply_cut parameter, so now without the else fallback they fail

FAILED invisible_cities/reco/peak_functions_test.py::test_build_peak_development[S2-40-True] - TypeError: 'NoneType' object is not callable FAILED invisible_cities/reco/peak_functions_test.py::test_find_peaks_s2_style - TypeError: 'NoneType' object is not callable FAILED invisible_cities/reco/peak_functions_test.py::test_get_pmap - TypeError: 'NoneType' object is not callable

Should we redo the functions that are using apply_cut = None so that there is some parameter passes through?

I think there are only two or three options, but correct me if I'm wrong:

None meant "no selection", so everything is kept. You would need to add this option to CutAlgo

None meant "threshold selection". In this case you need to adapt the tests by explicitly sending the original default threshold parameters.

None meant "threshold selection but with some (non-defaul) specific value of the parameters (e.g. thr=0). You would need to tweak the tests to specify these values as well.

None in this case means no cut is applied at all, I think the best option would be to implement this explicitly as an option.

I noticed that in the old version of the tests that failed above they set thr_sipm_s2 = -1, this is essentially equivalent to setting no threshold the way I see it.

If you agree I'll make an option for no threshold and that should fix things.

Yup. Go ahead.

Done. What do you think?

Also should I make these functions have CutAlgo.no_cut as the default option?

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR103

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR130

b268b32#diff-da42ca1aac462421c8d637d77af246ebd19d1c5c9d1764c251b771b420a1cacdR154

gonzaponte

A few more comments. I still need to check a few things, but at least you can make some progress while I do that.

gonzaponte · 2026-06-23T09:31:57Z

+    def apply_cutting_function(wfs):
+        return func(wfs)
+
+    return apply_cutting_function


this is the same as return func. you can also use the return statement directly in the if/elif branches.

gonzaponte · 2026-06-23T10:01:50Z

+    # assume that if the detector_db is None, you return sipm threshold as the number provided
+    if detector_db is None:
+        sipm_thr = thr_sipm
+    else:
+        # extract sipm threshold
+        sipm_thr = get_actual_sipm_thr(thr_sipm_type, thr_sipm, detector_db, run_number)


If you add comments, make them say something that is not obvious from the code. In this case, since it is more a description of the function overall (rather than an implementation detail or trick), I would use the docstring to explain what the function does.

gonzaponte · 2026-06-23T15:49:19Z

+    -------
+    2D array of shape (n_sipms, n_time_bins) containing the input waveforms with entries below threshold set to zero.
+    """
+    thr = to_col_vector(np.full(wfs.shape[0], zeroing_thr))


Suggested change

thr = to_col_vector(np.full(wfs.shape[0], zeroing_thr))

thr = np.full((wfs.shape[0], 1), zeroing_thr)

gonzaponte · 2026-06-23T15:52:37Z

+    # zero entries below threshold
+    zwfs = zero_wfs_below_threshold(wfs, zeroing_thr)
+
+    # returns selected ids and waveforms above integral
+    return select_wfs_above_time_integrated_thr(zwfs, integration_thr)


remove comments

gonzaponte · 2026-06-23T15:56:14Z

+def apply_circular_padding(selected_ids_no_isolated : np.ndarray,
+                           sipm_x                   : np.ndarray,
+                           sipm_y                   : np.ndarray,
+                           padding_radius           : float) -> np.ndarray:
+    """
+    Receives a list of SiPM IDs corresponding to the most energetic SiPMs clustered 
+    near the event. For these SiPMs, creates circular padding of radius padding_radius, 
+    selecting all SiPMs within that radius. Stores the union of all selected SiPMs. 
+    Outputs the SiPM IDs which are relevant for a given event.
+
+    Parameters
+    ----------
+    selected_ids_no_isolated : Boolean array of shape (n_sipms,) corresponding to the "relevant" SiPMs.
+    sipm_x                   : 1D array of shape (n_sipms,) containing the x positions of the SiPMs.
+    sipm_y                   : 1D array of shape (n_sipms,) containing the y positions of the SiPMs.
+    padding_radius           : Distance threshold in mm used to create circular padding around selected SiPMs.
+
+    Returns
+    -------
+    sipm_ids_with_signal : Boolean array of shape (n_sipms,) where True indicates that the SiPM is selected.
+    """


Whenever possible, try to frame the function in a more general way. Here, you use variable names and an overall function explanation that is perhaps easy to follow if you read the whole Pyrrha implementation, but not on their own. It should be possible to read and interpret this function without understanding Pyrrha as a whole.

Ian0sborne and others added 30 commits February 25, 2026 16:27

add sipm selection function using median and std

040d4ad

add sipm selection method using a charge threshold

eecf819

add sipm selection function using top n most energetic sipms

38364ff

add function that kills isolated sipms

57955f4

add function that creates circular padding around selected sipms

34ec2ef

add function that creates the overall sipm masks

a55132e

load detector geometry in

f137e23

remove thresholds from calibrate_sipms()

62b56aa

create CutAlgo() class

3e2f333

modify charge_threshold_method()

ff9346b

Done for backwards compatibility, this function now mirrors that of `calibrate_sipms()` before thresholding was removed.

Implement threshold_sipm_selection()

ea10ade

Include apply_cutting_function()

78dda04

This is the main controlling function, that decides which type of cut will be applied.

Implement apply_cut() into irene

9c91722

Implement apply_cut() into compute_and_write_pmaps()

b268b32

This included the adjustment of the cuts, applying `select_wfs_above_time_integrated_thr()` and the zeroing integration cut before the waveform rebinning.

Update default irene config

1ead58b

Remove whitespaces, add newline

d0c1c24

Allow calibrate_sipms() to apply threshold.

a675fcb

This is to avoid possible situations where `calibrate_sipms()` is used outwith irene. `thr` is now a keyword argument to ensure that if a threshold is applied it is done so with intent.

Include thr_sipm_s2 into cutting_params

bdb026e

Implement thr_sipm_s2 in charge_threshold_method()

1e5e88f

Implement integration cut based on slices

78cb788

Initially this was done over the entire waveforms due to the reordering of the cuts when applied in `build_sipm_responses()`. As suggested by @Ian0sborne, it would be wise to move these cuts out of `compute_and_write_pmaps()`

Adjust tests to account for cut_params

621bc79

Implement modifiable cuts in hypathia

dac33dd

Update docstrings, type annotation

7cfd743

add pyrrha cutting method as an option in CutAlgo and `apply_cuttin…

502559d

…g_function`

pass load_db parameters into make_sipm_selection function

5d5b786

add documentation to make_sipm_selection

04a0867

implement spatial_selection_method (previously named `make_sipm_sel…

4f243a3

…ection`) into `pyrrha_sipm_selection`

modify spatial_selection_method to return the ids of the selected S…

dfd3ba0

…iPMs and their waveforms

implement waveform index slicing in spatial_selection_method

f901531

ammend function to return sliced waveforms

6eb7b88

Ian0sborne added 8 commits March 4, 2026 17:41

revert back to full waveform

16147f5

implement SiPMSelectionMethod class to call specific selection meth…

146013e

…ods for pyrrha

modify pyrrha_sipm_selection and spatial_selection_method to use …

3fd5f14

…SiPMSelectionMethod class

include another method for initial SiPM selection and raise an error …

83fe914

…for incorrect input

remove time index dependency of SiPM cutting functions

300dfb1

remove select_wf_slices_above_time_integrated_thr

ab88fdb

update docstrings

76b961c

remove unused variable from hypathia

b7f45d4

Ian0sborne force-pushed the implement_pyrrha branch from fb435dd to b7f45d4 Compare May 7, 2026 15:30

Ian0sborne commented May 7, 2026

View reviewed changes

Ian0sborne added 11 commits May 7, 2026 16:53

raise error for incorrect cutting function input

be156fe

add fixture and test for charge_threshold_method()

6dd6bab

add test for charge_threshold_method()

b920832

simplify naming

75aa579

add test for top_n_method

5ce0864

add fixture and test for kill_isolated_sipms()

ebbe61a

amend apply_circular_padding() to not kill SiPMs when padding is se…

51c2e63

…t to 0

add test for apply_circular_padding() and modify fixture to accomod…

021e6ef

…ate for it

add zero_wfs_below_threshold() to zero wavefrom entries below a thr…

58b88aa

…eshold outside of `charge_threshold_method()`

update calibrate_sipms() and corresponding test it to rely on `zero…

bd1b9d9

…_wfs_below_threshold()` for zeroing waveform entries

add test for zero_wfs_below_threshold() with small tweak in fixture

66e94a7

Ian0sborne commented May 11, 2026

View reviewed changes

Comment thread invisible_cities/calib/calib_sensors_functions.py Outdated

gonzaponte requested changes Jun 4, 2026

View reviewed changes

Ian0sborne added 6 commits June 5, 2026 11:24

Change comment position

dd0533c

Update SiPMCalibMode name to match new functionality

5a9f871

Remove default parameters in wfm_functions.py

157639f

Update & improve docstrings

325cd75

Remove trailing underscores

0c64672

Implement no_cut option in cut algorithms

e692475

gonzaponte requested changes Jun 23, 2026

View reviewed changes

	thr = to_col_vector(np.full(wfs.shape[0], zeroing_thr))
	thr = np.full((wfs.shape[0], 1), zeroing_thr)

Uh oh!

Conversation

Ian0sborne commented Mar 5, 2026

Uh oh!

Ian0sborne May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gonzaponte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gonzaponte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ian0sborne May 7, 2026 •

edited

Loading