WireCell / wire-cell-toolkit

Toolkit for Liquid Argon TPC Simulation and Reconstruction
https://wirecell.github.io/
Other
7 stars 22 forks source link

refresh nticks for each event #317

Open wenqiang-gu opened 4 months ago

wenqiang-gu commented 4 months ago

In ProtoDUNE HD, the lengths of raw::RawDigits are not fixed among different events. Currently, the OmnibusNoiseFilter will determine "nticks" at the beginning of configuration and will not update it later. The purpose of this PR is to refresh "nticks" for each event.

To use it, just add reload_nticks: true in the nf.jsonnet (the default value is false).

   local obnf = g.pnode({
        type: 'OmnibusNoiseFilter',
        name: name,
        data: {

            // Nonzero forces the number of ticks in the waveform
            nticks: 0,
            reload_nticks: true,
brettviren commented 4 months ago

Locally, this looks reasonable to me, @wenqiang-gu . In hindsight, we should have started out with this assumption of variable length signals...

It seems to me that the reload_nticks (as a configuration parameter) is redundant with the configuration nticks=0. Of course we still need an internal bool to flag the operational mode. If the configuration is truly redundant, it is best to remove reload_nticks (internally test on initial nticks==0) as the result will be more robust against user config errors.

I do have a concern for possible side-effects in the actual noise filters.

First, are there any noise filters which are configured with their own fixed nticks value and if so, how do they react to input signals with differing sizes?

Second, the harmonic filter(s) at some point (still true?) were configured based on frequency bins and not frequency ranges. Specifying with bins locks them into only working properly when given their expected, fixed size signal. They should be modified to be configured with frequency ranges and to recalculate the bins to mask anytime an input nticks changed. For backwards compatibility, they could still accept (optional/depreciated) config forms which are in the current terms of bins and nticks but convert that and store that info as frequency range.

Please check if there are any cases like this where the user will get a surprise. If it happens that PDHD does not use any of these problematic filters, then at least please make a new ticket that lists the work needed to fix these problematic filters.