Add a new grouped filter in OmnibusNoiseFilter

[wenqiang-gu]

While Xuyang is developing noise filters for ProtoDUNE HD, a new type of "nosie" occurs in half of FEMB with large negative pulses. Currently, the coherent noise filter in PDHD bundles all 40 U channels from a FEMB in a group. And similarly, all 40 V channels from a FEMB, all 48 W channels from a FEMB. Therefore, these two grouping policies are different.

Here is how the coherent noise filter is implemented in OmnibusNoiseFilter:

https://github.com/WireCell/wire-cell-toolkit/blob/1a00d641b90d500238c4696bc204f45905c2a454/sigproc/src/OmnibusNoiseFilter.cxx#L187

I would like to add a different filter, e.g.,

for (auto group: m_noisedb->partial_femb() ) {

...
        for (auto filter : m_partial_femb) {
            auto masks = filter->apply(chgrp);
            Waveform::merge(cmm, masks, m_maskmap);
        }
...
}

Would this proposal be align with the overall design of wire-cell-toolkit?

[wenqiang-gu]

@brettviren Could you comment on this proposal?

Also, could you add Xuyang (GitHub username: Ningclover)as a core developer in GitHub? She needs to develop this filter.

[brettviren]

Hi @wenqiang-gu

Almost, but I think not quite. We should not see a string like "femb" inside WCT C++ as that is a detector-specific idea.

To refactor the problem and your proposal more generically: right now WCT accepts only a single association between a single "set of channel groups" ({CG}) and a single "set of group filters" ({GF}):

 {CG} <--> {GF}

What we want is to extend this to allow multiple associations:

 {CG1} <--> {GF1}
 {CG2} <--> {GF2}
 {CG3} <--> {GF3}

Just to define a label, I call this: "multi group coherent channels" (MGCC). We can then say that WCT currently only supports "single group coherent channels" (SGCC).

To add support for "MGCC" we must keep in mind backwards compatibility. Also, I think it is best to minimize the footprint (amount of code change) of the extension.

So, I suggest we do the following:

1. Ignore OmniChannelNoiseDB and focus only on OmnibusNoiseFilter.
2. Remove the current "SGCC loop" in OmnibusNoiseFilter:

   for (auto group : m_noisedb->coherent_channels()) {
      ...

3. Replace the SGCC loop with a new MGCC loop. It will be similar but instead iterates over MGCC information (I'll describe). This will need a data member on OmnibusNoiseFilter like std::vector<MGCC> m_multigroup_coherent) to provide ready-to-use representation of MGCC. The new MGCC loop would then look something like:

   for (const auto& [mgcc] : m_multigroup_coherent)
       for (auto ch : mgcc.channels) {
           // fill chgrp as in the SGCC loop
       }
       for (auto filter : mgcc.filters) {
           // apply filter as in the SGCC loop
           auto masks = filter->apply(chgrp);
           Waveform::merge(cmm, masks, m_maskmap);
       }
       // ... etc as the SGCC loop

4. Introduce a new configuration attribute to OmnibusNoiseFilter that would allow Jsonnet like:

local filter1 = /* usual IChannelFilter config */;
local filter2 = /* usual IChannelFilter config */;
type: 'OmnibusNoiseFilter',
data: {
  multigroup_coherent: [
    {
      channels: [list of channel IDs],
      filters: [wc.tn(filter1), wc.tn(filter2)]
     },
     { ... },
     { ... },
     { ... },
  ],
}, // ...

5. Define new little struct called MGCC inside OmnibusNoiseFilter like:

   struct MGCC {
   std::vector<int> channels;
   std::vector<IChannelFilter::pointer> filters;
   };

6. Construct the m_multigroup_coherent in OmnibusNoiseFilter::configure() from two pieces of information. First, fill it with the SGCC information from m_noisedb->coherent_channels() and m_grouped. Second, fill it from information from the multigroup_coherent configuration attribute.

Note, I have intentionally excluded OmniChannelNoiseDB because I believe MGCC information is only required inside OmnibusNoiseFitler. If that is wrong, we may need to extend both OmnibusNoiseFitler and OmniChannelNoiseDB.

Also, I wrote the above so that MGCC support is a superset that covers SGCC. That is, we may - in principle - (re)move existing SGCC configuration and have the same functionality expressed as MGCC. It is possible I missed something that would not allow this design to be a superset of SGCC functionality. If so we should fix the design to allow the superset. And, I don't mean to imply we must remove SGCC config, just that it should be possible. Existing experiments may not want to change. But, new configuration can be made simpler.

Sorry this is long, but I hope it makes sense. We can of course discuss.

(And, I'll invite Xuyang).

[wenqiang-gu]

It sounds good to me. In fact, I think we can even totally remove OmnibusNoiseDB from OmnibusNoiseFilter as the current usage of OmnibusNoiseDB is just: m_noisedb->bad_channels() and m_noisedb->cohenrent_channels(). Once we implement MGCC, we can pass those channels from the jsonnet level, e.g.,

type: 'OmnibusNoiseFilter',
data: {
  multigroup_coherent: [
    {
      channels: chndb.groups;
      filters: [wc.tn(filter1), wc.tn(filter2)]
     },
    {
      channels: chndb.bad;
      filters: [wc.tn(filter1), wc.tn(filter2)]
     },
     { ... },
     { ... },
  ],
}, // ...

[wenqiang-gu]

I think we can even totally remove OmnibusNoiseDB from OmnibusNoiseFilter

Never mind, I recall you mentioned backwards compatibility.

I am working on the implementation now. PS, can we name the struct MGCF (multi-group channel filters)? "coherent" seems a bit jargon-heavy.

[wenqiang-gu]

Hi @brettviren , I submitted my PR: https://github.com/WireCell/wire-cell-toolkit/pull/328

It's implemented by following what you proposed except that: 1) MGCC -> MGCF 2) use vector<vector<int>> in MGCF

            struct MGCF {
                std::vector<IChannelNoiseDatabase::channel_group_t> channelgroups;
                std::vector<IChannelFilter::pointer> filters;
            };

[brettviren]

Looks good, @wenqiang-gu . And, yes v<v<i>> is correct and the naming is good.

I reviewed the PR. It looks good. Feel free to merge if/when you are ready.

---

An aside about removing CNDB from NF.

I do like your idea about removing CNDB entirely from NF as it would simplify configuration substantially. But of course, it would be a "breaking change".

If WCT releases also included all experiments' config or otherwise we would release experiments' configuration tied to C++ releases, we could make such changes easily.

But since we do not have this level of control on the configs we must introduce breaking changes by following some kind of "deprecation procedure". This procedure would span several releases amd communicate our intention to the users. The procedure would have at least two stages.

1. Immediately we may add a log.warn("Use of CHND is deprecated and will be removed in the future") to print by NF if it is configured to use a CNDB object. Besides the warning, the code works as normal (eg, as in your PR).
2. We pick a future "major release" of WCT to actually remove support for NF to use a CHND. The release note details the "breaking change" and give users guidance how to upgrade their configuration. To be friendly, we keep the check for any CNDB being configured but change it from warning to critical error: log.critical("Use of CNDB is obsolete, see notes for release X.Y.Z'); raise<ValueError>("unexpected configuration: CNDB"). This will catch new code using old config. To be super friendly, maybe the release includes a script to help the user to convert old config to new config.

In any case, we should not couple this possible cleanup with your PR here. We can discuss to see if the cleanup of removing CNDB is worth the effort.

[wenqiang-gu]

Thank you for the comment. I added a warning for later reference:

log->warn("Use of CHND is deprecated and will be removed in the future.”);