Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RmsFiller tools missing in vertical-drift reco #22

Open
dladams opened this issue Mar 1, 2023 · 13 comments
Open

RmsFiller tools missing in vertical-drift reco #22

dladams opened this issue Mar 1, 2023 · 13 comments
Assignees

Comments

@dladams
Copy link
Contributor

dladams commented Mar 1, 2023

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
Copy link
Contributor Author

dladams commented Mar 1, 2023

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 added a commit that referenced this issue Mar 1, 2023
@dladams
Copy link
Contributor Author

dladams commented Mar 1, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

dladams commented Mar 2, 2023

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
Copy link
Contributor Author

dladams commented Mar 3, 2023

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

@wenqiang-gu
Copy link
Contributor

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
Copy link
Contributor Author

dladams commented Mar 14, 2023

I am looking into this. --david

@dladams
Copy link
Contributor Author

dladams commented Mar 14, 2023

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
Copy link
Contributor

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

dladams added a commit that referenced this issue Mar 14, 2023
@dladams
Copy link
Contributor Author

dladams commented Mar 14, 2023

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
Copy link
Contributor Author

dladams commented Mar 14, 2023

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
Copy link
Contributor Author

dladams commented Mar 14, 2023

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants