SBNSoftware / icaruscode

Main/top level repository for ICARUS specific code
11 stars 33 forks source link

Add a CRT Hit flag to categorize side CRT Z position reconstruction #738

Closed aheggest closed 3 months ago

aheggest commented 3 months ago

This PR adds a CRT Hit flag to help identify some cases where the Z-position reconstruction for side CRT hits isnt so good. The Side CRT Hit are made of coincidences between inner and outer layers, with two layers being readout on opposite ends by two different FEBs (For East and West Side CRT walls only, North and South Side CRT walls use cut modules and readout on a single end). In some cases, the Side CRT Hit Z position can be reconstructed off the bounds of the module wall due to the FEB readouts on opposite ends being too far apart in time (T0 timestamps on opposite ends of the module are >= 50 ns apart, 50 ns ~ 800 m (full module length) x 0.062 ns/cm (prop delay)). These CRT Hits are flagged with flag = 4. We also see a large collection of CRT Hits with a final Z defaulted to the exact center of the module from single ended readouts on the same end of the module in both layers, which are flaged with flag = 5. I also flag CRT Hits that had timestamps on opposite ends >= 50 ns but are still reconstructed within module bounds with flag = 3. This PR flags the badly reconstructed side CRT Hits (with flags 4 and 5) so the analyzer can choose whether or not to remove those CRT Hits, which one might want to do if the analysis is CRT position dependent. If wanting to be extra conservative, one might also want to remove CRT Hits with flag = 3.

mmrosenberg commented 3 months ago

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_90_00

FNALbuild commented 3 months ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild commented 3 months ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild commented 3 months ago

:warning: CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

:rotating_light: For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild commented 3 months ago

:x: CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

:rotating_light: For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

francescopoppi commented 3 months ago

Hi @aheggest thanks for this PR. I think overall this looks good, but I have some general suggestions and some request we should address before going to the very details. First of all, using a variable of a different type (double vs int) and with a different name (ts0_corr for a variable which should be called something along the line of "flags") is very misleading, prone to error and, especially for the usage of double for an int, its fairly wrong. Here comes my first request: let's add a new member to the CRTHit obj in sbnobj which specificallt does what you are asking.I propose to accompany this with an sbnobj PR which adds the variable and lets ask SBND people (Henry Lay for example) to review the sbnobj pull-request. This will come with some work to adjust icaruscode to the new variable. I think This is important because ICARUSCODE is already in an unpleasent state of "patches" lying around which have been introduced by members that left the collaboration and now are set on stone. I would volounteer to help you in fixing here and there in icaruscode the relevant part for the flag variable inclusion. My second observation is that by doing so, you are only adding the flag variable to Calibration Ntuple, was this intended? I would have expected to have this propagated to all the modules that are using the CRT hits, for example CAFs. Let me know what you think of it!

francescopoppi commented 3 months ago

As an additional question: would you be willing to add to this PR also the reconstructed position in case 5 and 6 when using the PE-distance conversion? Do you have an estimate about how much time would it require for this study to be finalized? This would help us understand if we can add everything in a "Side CRT position improvement" PR or we need to split it in a couple or more PR

aheggest commented 3 months ago

thank you @francescopoppi for your feedback! I've decided to close this PR while I try to figure out the best way to improve the reconstruction