-
Notifications
You must be signed in to change notification settings - Fork 15
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
Deconvolution and OphitFinder #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I have included a few comments throughout the files.
duneopdet/OpticalDetector/Deconvolution/Deconvolution_module.cc
Outdated
Show resolved
Hide resolved
duneopdet/OpticalDetector/Deconvolution/Deconvolution_module.cc
Outdated
Show resolved
Hide resolved
duneopdet/OpticalDetector/Deconvolution/Deconvolution_module.cc
Outdated
Show resolved
Hide resolved
out_recob_float.resize(OriginalWaveformSize); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a brief explanation of what the letters mean?
double fLineNoiseRMS; //!< Pedestal RMS in ADC counts | ||
size_t fPreTrigger; //!< In ticks | ||
std::vector<double> fSinglePEWaveform; //!< Template for a single PE in ADC | ||
double fSinglePEAmplitude; //!< single PE amplitude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only take the fhicl parameter if no template file is provided, right? If so, maybe include this in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fSinglePEWaveform is a vector that stores the single PE template, this is needed to calculate the spe response spectral density. SinglePEAmplitude is a vector that stores the maximum amplitude of the single PE, we use this for found the maximum amplitude of the waveform in terms of PE.
I've relocated the definition of the vector in the module to avoid confusion and included this description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Maritza, this is fantastic work! I would say that some of these modules would be worth having in the general LArSoft repositories. Since this new OpHitFinder can run also with old raw::OpDetWaveforms, I would update the old OpHitFinder with this new one. I have added a few comments mainly to better describe the modules and how to use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this module needed? cannot OpFlashAna_module be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This analyzer module is based on opflashana but does not include an analysis of opflash, so far we have analyzed ophits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the user is interested in the analysis of ophits, OphitAna or OpFlashAna could be adapted. Should we consider removing OpHitFindAna_module and its fchl from duneopdet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked offline, and @maridelg saw that OpHitAna depends on OpDigiProperties, which we deleted, and OpFlashAna requires the existence of flashes. However, I think that just moving the flash-related code inside an if statement in OpFlashAna would do the work:
if(fMakeFlashHitMatchTree || fMakeFlashBreakdownTree || fMakePerFlashTree || fMakePerEventFlashTree || fMakePerFlashHists || fMakeFlashPosHist || fMakeFlashTimeHist)
And we would avoid be maintaining replicated code in the future.
Do you see any problem with that, @maridelg?
int channel = decowaveform.Channel(); | ||
|
||
// Implement different end time for waveforms of variable length | ||
double startTime = waveform.TimeStamp() - firstWaveformTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are reading the raw::Waveforms just to take the TimeStamp... I see in the class definition that "The time is expected to be the same as for the raw::OpDetWaveform
that originates it". However, in Raw::OpDetWaveform, the time is TimeStamp_t, while in recob::OpWaveform, the time is raw::RDTimeStamp. Is this the reason of the problem? Maybe we can ping Tingjun to ask why the timestamps have a different format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, this is the problem.. The OpWaveform recob is designed based on recob::Wire (same structure and parameters), it was created for the TPC. According to Tom Junk, the RDTimeStamp was created as a plugin to raw::RawDigit to have an absolute timestamp channel-by-channel. I think we will adapt recob OpWaveform more to the needs of the optical detector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end, what happened with this? Is recob::OpWaveform being adapted? I see the code stills reads raw::OpDetWaveform just to take the TimeStamp doubling the memory consumption in DecoAnalysis but also in the HitFinder.
Hi Laura and Jose, |
@@ -24,4 +24,12 @@ standard_ophit_finder_spe: | |||
|
|||
} | |||
|
|||
# If set to "raw", use the below configuration: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the standard_ophit
inheriting its configuration from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
standard_ophit comes from opticaldetectormodules.fcl which lives in Larana, and I have kept the values with which I get better results with raw waveforms. Do you agree or keep the default values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard ophit configuration actually lives here: opticaldetectormodules_dune.fcl. If you need to change those values, please do it there. But keep in mind that we may want to keep for a while the simple PE config too so you may want to create a separate configuration if those numbers are to be good for something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Laura for your suggestions.I have added two proposals.
@@ -24,4 +24,12 @@ standard_ophit_finder_spe: | |||
|
|||
} | |||
|
|||
# If set to "raw", use the below configuration: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
standard_ophit comes from opticaldetectormodules.fcl which lives in Larana, and I have kept the values with which I get better results with raw waveforms. Do you agree or keep the default values?
Samples: 1000 # Timewindow (ReadoutWindow) in [ticks] | ||
Scale: 0.001 # Scaling of resulting deconvolution signal. | ||
DigiDataColumn: 0 # SPE template source file column. | ||
DigiDataFile: "fbk_decon_digi_2023.txt" # The SPE template with undershoot and without pretrigger (in ADC*us), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think here we could include both configurations (FBK and HPK) by doing something similar to what I proposed for the filters:
dune_deconvolution_fbk:
{
(...)
DigiDataFile: "fbk_decon_digi_2023.txt" # The SPE template with undershoot and without pretrigger (in ADC*us),
# was obtained from DAPHNE V2 and the cold amplifier with 48 SiPM FBK.
(...)
}
dune_deconvolution_hpk: @Local::dune_deconvolution_fbk
dune_deconvolution_hpk.DigiDataFile: "hpk_decon_digi_2023.txt" # The SPE template with undershoot and without pretrigger (in ADC*us),
# was obtained from DAPHNE V2 and the cold amplifier with 48 SiPM HPK.
dune_deconvolution: @Local::dune_deconvolution_fbk
Would that make sense (assuming all the other parameters are the same for HPK and FBK)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like, we can do it !!
Hello Laura and Jose, Thanks a lot for your comments and suggestions. I have made a new commit with the suggested changes. I've checked all the files in the latest version of LarSoft (v09_72_01) and everything works fine. I have fixing three modules that had the error related to_ Thanks and I stay tuned to your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Maritza, thank you for your new commit. I have added a few comments. Please reach out in case of doubts.
|
||
} | ||
|
||
//DataFile hpk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave the lines that define the hpk deconvolution without the hash or were you getting problems here?
evt.getByLabel(fInputModule, OpHitHandle); | ||
|
||
// Access ART's TFileService, which will handle creating and writing | ||
art::ServiceHandle<art::TFileService const> tfs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in line 107
fInputModule = pset.get< std::string >("InputModule"); | ||
fInstanceName = pset.get< std::string >("InstanceName"); | ||
|
||
art::ServiceHandle<art::TFileService const> tfs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in line 153
@@ -24,4 +24,12 @@ standard_ophit_finder_spe: | |||
|
|||
} | |||
|
|||
# If set to "raw", use the below configuration: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard ophit configuration actually lives here: opticaldetectormodules_dune.fcl. If you need to change those values, please do it there. But keep in mind that we may want to keep for a while the simple PE config too so you may want to create a separate configuration if those numbers are to be good for something else.
76ae296
to
edc5058
Compare
@maridelg, why was this PR closed? The SciSoft team is trying to analyze LArSoft/lardataobj#37, and it appears to be related to this PR. |
@knoepfel, we merged it to the main branch, since we needed this code even it was not optimized. |
This PR was actually replaced by this other one #30. |
The PR for the new Deconvolution module and the new version of OpHitFinder where the recob::OpWaveform object has been included.