-
Notifications
You must be signed in to change notification settings - Fork 54
Add lightpropagationcorrection module #813
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
Conversation
|
hi @asanchezcastillo , just snooping around here, is there any reason we wouldn't want OpT0Finder to inherit the corrected OpFlashes? |
|
Hi @lynnt20 ! I have chosen BFM as the default option because both FMs have shown very similar matching efficiency with a handscanned neutrino sample (https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=42327), which translates into a very similar timing reconstruction performance (I can share the plots in private). Furthermore, since the BFM is model-independent I believe it will be less affected by any changes that may happen in the future to our simulation. |
fjnicolas
left a 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.
@asanchezcastillo this looks great and will be extremely useful to have for the next production!
I spotted a few unused functions/variables that could be dropped. The two more critical things:
- SPEC TDC is a must to the module, so it shouldn't be configurable
- There may be some conflict with the ReadoutDelay variable?
In addition to that, I think it may be useful to also save OpFlash t0 without any correction at all (right now it has the light propagation time using eta_PMT by default). In that way we could isolate each of the constributions to the timing correction at the analysis level. That would me adding an additional member to the PR in sbnobj though. I know we are in a tight schedule, so this request should NOT prevent this PR moving forward into production.
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.cc
Outdated
Show resolved
Hide resolved
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.cc
Outdated
Show resolved
Hide resolved
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.cc
Outdated
Show resolved
Hide resolved
| double sbnd::LightPropagationCorrection::GetFlashT0(double flash_time, std::vector<recob::OpHit> ophitlist){ | ||
|
|
||
| std::vector< std::pair<double, double> > selected_hits; | ||
| double pe_sum = 0; | ||
|
|
||
| // fill vector with selected hits in the specified window | ||
| for(auto const& hit : ophitlist) { | ||
| if( hit.PeakTime()<flash_time+fPostWindow && hit.PeakTime()>flash_time-fPreWindow && hit.PE()>fMinHitPE){ | ||
| selected_hits.push_back( std::make_pair(hit.PE(), hit.PeakTime())); | ||
| pe_sum += hit.PE(); | ||
| } | ||
| } | ||
|
|
||
| if(pe_sum>0){ | ||
| // sort vector by number of #PE (ascending order) | ||
| std::sort( selected_hits.begin(), selected_hits.end(), std::greater< std::pair<double, double> >() ); | ||
|
|
||
| double flasht0_mean=0., pe_count=0.; | ||
| int nophits=0; | ||
|
|
||
| // loop over selected ophits | ||
| for (size_t ix=0; ix<selected_hits.size(); ix++) { | ||
| pe_count += selected_hits[ix].first; | ||
| flasht0_mean += selected_hits[ix].second; | ||
| nophits++; | ||
| if( pe_count/pe_sum>fPDFraction ) break; | ||
| } | ||
| return flasht0_mean/nophits; | ||
| } | ||
| else | ||
| return flash_time; | ||
| } |
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.
Do you need this function? You are using the one from the flash tools
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.
Indeed! Thanks for the catch:)
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.hh
Outdated
Show resolved
Hide resolved
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.hh
Outdated
Show resolved
Hide resolved
| if(_sliceMaxNuScore<fNuScoreThreshold){ | ||
| ResetSliceInfo(); | ||
| continue; // Skip to the next slice if the nu score is below threshold | ||
| } |
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.
Is this cut really needed here? Why not letting this module do the corrections for all the neutrino slices and let the analyzer place the cut?
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 main reason behind this choice is the computing time required to run over all the slices. Placing the cut here avoids running over slices that are clearly not neutrinos, which alleviates computing time.
sbndcode/LightPropagationCorrection/job/sbnd_lightpropagationcorrection_config.fcl
Outdated
Show resolved
Hide resolved
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.hh
Outdated
Show resolved
Hide resolved
sbndcode/LightPropagationCorrection/LightPropagationCorrection_module.cc
Show resolved
Hide resolved
JosiePaton
left a 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.
I see Fran has left a lot of good comments here, but for the sake of time I'll add my approval now for the change to the Standard Record pending discussion of his suggested changes :)
…_module.cc Co-authored-by: Francisco Javier Nicolás-Arnaldos <[email protected]>
|
@fjnicolas thanks a lot for the thorough review! I have removed the unused variables as well as the readout delay correction, which is not needed at this point. Given the tight schedule, I'll incorporate the modifications to the module once the production release is cut. |
fjnicolas
left a 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.
Thanks for the quick turnaround! All the changes look ok. Happy to work on another iteration for a future patch.
|
@asanchezcastillo Let me know if that merge commit looks incorrect. There was a conflict, but it seemed pretty simple so I resolved it. |
|
trigger build ci_ref=v10_10_02 LArSoft/lar*@LARSOFT_SUITE_v10_10_03 SBNSoftware/sbndaq_artdaq_core@v1_10_06 SBNSoftware/sbnd_data@v01_36_00 SBNSoftware/sbncode@v10_10_03 |
|
❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
trigger build ci_ref=v10_10_02 LArSoft/lar*@LARSOFT_SUITE_v10_10_03 SBNSoftware/sbndaq_artdaq_core@v1_10_06 SBNSoftware/sbnd_data@v01_36_00 SBNSoftware/sbncode@v10_10_03 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
Description
This PR implements a new module for applying light propagation corrections to OpFlash timing based on TPC information. It does select the OpFlash matched to every slice and correct the time of the flash based on the space points in the slice. After the correction, a new object
CorrectedOpFlashis created as well to store the new timing information, namely:This new object is also associated to its father slice and OpFlash. The module supports the use of the OpT0Finder and BarycenterFM tools for initial charge/light matching though the latter is used by default as it is a model-independent tool.
The module is included on
reco2_data.fclto be run as a part of the standard reconstruction workflow.This PR requires merging of the following PRs:
#807
SBNSoftware/sbnobj#140
SBNSoftware/sbnanaobj#157
SBNSoftware/sbncode#566
Checklist
Reviewers,AssigneesDevelopementRelevant PR links (optional)
Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)?
Link(s) to docdb describing changes (optional)
Is there a docdb describing the issue this solves or the feature added?
https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=42327