DUNE / dunedataprep

Apache License 2.0
0 stars 7 forks source link

RmsFiller tools missing in vertical-drift reco #22

Open dladams opened 1 year ago

dladams commented 1 year ago

Wenqiang report that a tool is missing in vertical-drift reco:

Hi David, I am still having the problem of CNR for the CRP3 coldbox data. Let me repeat the issue here (dunesw v09_67_00_d00):
lar -n 1 -c crp3cb_data_oct2022_reco.fcl /dune/data/users/wgu/crp3/1727_66_b_cb.test
Warning message is like: CnrGroupWeighted::getWeights: WARNING: Channel 54 does not have attribute rawRms
In the email, you suggest that I can switch the order of the tools for adcSampleFiller and vdtcb_adcChannelRawRmsFiller. So I did that in a local copy of fhicl (/dune/data/users/wgu/crp3/test/vdcb_dataprep_sequences.fcl). Here is the result when I dumped the fhicl:
  RawDigitPrepService: {
   CallgrindToolNames: []
   DoWires: true
   LogLevel: 3
   ToolNames: [
     "digitReader",
     "adcSampleFiller",
     "vdtcb_adcChannelRawRmsFiller",
     "vdtcb2_cnrw",
     "adcKeepAllSignalFinder",
     "vdcb_RemoveBadChannels"
   ]
   service_provider: "ToolBasedRawDigitPrepService"
  }
However, now I got an error message:
ToolBasedRawDigitPrepService::ctor:   Found tool adcSampleFiller @ 0xce87be0
ToolBasedRawDigitPrepService::ctor:   Fetching vdtcb_adcChannelRawRmsFiller
DuneToolManager::getPrivate: ERROR: No such tool name: vdtcb_adcChannelRawRmsFiller
ToolBasedRawDigitPrepService::ctor: ERROR: Unable to retrieve display tool vdtcb_adcChannelRawRmsFiller
ToolBasedRawDigitPrepService::ctor:   Fetching vdtcb2_cnrw
Do you have any idea? (edited) 
dladams commented 1 year ago

Looking in dunedataprep/DataPrep/fcl at vdcb2_tools.fcl and vdcb_dataprep_sequences, I do the tool vdcb_adcChannelRawRmsFiller is defined but the sequences use vdbcb_adcChannelRawRmsFiller and vdtcb_adcChannelRawRmsFiller. I modified the sequence to instead use the first tool and dropped a line which had no effect in the tools file.

dladams commented 1 year ago

The fix is committed. Wenqiang, can you check that all looks OK if you work with the head of this package (dunedataprep) or in the next release? We should check both crp2 and crp3 reco. Thank you.

wenqiang-gu commented 1 year ago

Okay, I will take a look for the upcoming new release.

Here are the waveforms from VD CRP3 coldbox, which are supposed to have coherent noise removed:

image

Left is the result with a typo in the configuration. After fixing the typo, the result looks good now.

dladams commented 1 year ago

BTW, I checked and the crp2 and crp3 configs only differ in channel status:

duneproc> fcldump crp2cb_data_jul2022_reco.fcl 5 >crp2cb_data_jul2022_reco.fcldump
duneproc> fcldump crp3cb_data_oct2022_reco.fcl 5 >crp3cb_data_oct2022_reco.fcldump
duneproc> l crp*dump
193 -rw-r--r--. 1 dladams fnalgrid 197040 Mar  2 14:22 crp2cb_data_jul2022_reco.fcldump
193 -rw-r--r--. 1 dladams fnalgrid 196998 Mar  2 14:23 crp3cb_data_oct2022_reco.fcldump
duneproc> diff crp*dump
1c1
< /home/dladams/proc/build/dev01/workdir/localProducts_dunesw_v09_68_00d00_e20_prof/dunesw/v09_68_00d00/fcl/crp2cb_data_jul2022_reco.fcl
---
> /home/dladams/proc/build/dev01/workdir/localProducts_dunesw_v09_68_00d00_e20_prof/dunesw/v09_68_00d00/fcl/crp3cb_data_oct2022_reco.fcl
8c8
< process_name: "CRP2CBDataReco"
---
> process_name: "CRP3CBDataReco"
1635c1635
<     NoisyChannels: [263, 264, 265, 266, 1297, 1298, 2667, 2668]
---
>     NoisyChannels: []
duneproc>             
dladams commented 1 year ago

We should close this as soon as we have positive feedback here or when dunesw issue 53 is closed.

wenqiang-gu commented 1 year ago

Hi @dladams , the new release v09_69_00d00 has included this fixing. However, we noticed that there is always a "Segmentation fault" at the end of a reco job for coldbox data. Since v09_69_00d00 has some other commits, I have again tested based on v09_68_00d00 and with a minimal change to "vdcb_dataprep_sequences.fcl". I can reproduce the "segmentaion fault". Could you take a look at this issue? Could it be a memory issue? Here is my command:

lar -n 1 -c crp3cb_data_oct2022_reco.fcl /dune/data/users/wgu/crp3/1727_66_b_cb.test
dladams commented 1 year ago

I am looking into this. --david

dladams commented 1 year ago

I also see a crash. It occurs in C++ cleanup in the AdcChannelStatus dtor when the channel status service is accessed through a bare pointer. Presumably the latter object has already been destroyed and I laid the trap for myself with this code in the ctor:

    m_pChannelStatusProvider = &art::ServiceHandle()->GetProvider();

I will try holding the service handle instead of (or in addition to) the bare pointer.

I have added Kyle who may be able to confirm this is the right approach or suggest another.

knoepfel commented 1 year ago

@dladams, can you send me a link to the code in question?

dladams commented 1 year ago

The code is here but, after thinking a bit, I realize the channel status service should not be used during cleanup since we are not inside of an event. A crash isn't a very nice way to advertise that but good to know the code was doing something wrong.

I fixed the problem by zeroing the pointer in the dtor. I no longer get the crash.

dladams commented 1 year ago

The fix is pushed. Please test and report back here but I am confident the problem has resolved. I will also have a peak at the config. We probably don't want to be making any plots much less summary plots in this context.

dladams commented 1 year ago

The tool was originally written to make plots of metric (here RMS) vs. channel and makes the graphs even if not configured to print to write out the graphs. The feature to write the metric to the (transient) event metadata was added later. That is the feature we need here. Other that wasting a few cycles and little memory, there should be no harm in using the code as it is.

If you don't want update to use the new code (now pushed to dunedataprep develop), the crash can also be avoided and the number of graphs reduced by a factor four by setting

  tools.vdcb_adcChannelRawRmsFiller.PlotUsesStatus: 0
knoepfel commented 1 year ago

The code is here but, after thinking a bit, I realize the channel status service should not be used during cleanup since we are not inside of an event. A crash isn't a very nice way to advertise that but good to know the code was doing something wrong.

Ah, I see. Services are destroyed after the art source and art modules...which means the tools are probably owned by a statically allocated object somewhere. Yes, that's fragile.