Integrate detector hits with larsoft#2278
Integrate detector hits with larsoft#2278sethrj wants to merge 46 commits intoceleritas-project:developfrom
Conversation
Test summary 5 950 files 9 578 suites 21m 16s ⏱️ Results for commit 886a3cd. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2278 +/- ##
===========================================
- Coverage 87.24% 87.23% -0.02%
===========================================
Files 1357 1357
Lines 43246 43292 +46
Branches 13189 13212 +23
===========================================
+ Hits 37731 37766 +35
- Misses 4307 4315 +8
- Partials 1208 1211 +3
🚀 New features to boost your workflow:
|
sethrj
force-pushed
the
larsoft-detector-hit
branch
from
1cb635c to
b7ba35a
Compare
sethrj
force-pushed
the
larsoft-detector-hit
branch
from
156b4ca to
21dc62c
Compare
Co-authored-by: Amanda Lund <alund@anl.gov>
sethrj
force-pushed
the
larsoft-detector-hit
branch
from
3cc3221 to
9774646
Compare
|
@stognini Could you review? Thanks! |
stognini
left a comment
stognini
left a comment
There was a problem hiding this comment.
Looks great. Thanks!
There are 2 issues I am investigating that we'll fix on a future PR:
SimEnergyDepositsoutside LAr (and thus crashing the execution)- The
PDSimAnamodule (from my branch, which I'll open a PR soon), generates reasonableOpDetBacktrackerRecordplots fromPDFastSimbut ourPDFullSimCelerOBTRs are empty, even though we are transporting photons.
|
@stognini Actually I want to get the dune cryostat geometry working and producing hits before I merge this, so it'll be just another minute. |
sethrj
force-pushed
the
larsoft-detector-hit
branch
from
17785a4 to
0fbf69a
Compare
Assisted-by: GitHub Copilot (Claude Sonnet 4.5)
Assisted-by: GitHub Copilot (Claude Sonnet 4.6)
| { | ||
| return type != GeneratorType::size_ && num_photons > 0 | ||
| && step_length > 0 && material && continuous_edep_fraction >= 0 | ||
| && step_length > 0 && continuous_edep_fraction >= 0 |
There was a problem hiding this comment.
@amandalund I remembered after I 'fixed' this that we'd previously discussed the pros and cons here: I think the benefits of "ensuring" that the provided data is enough to send to Celeritas outweighs the disadvantage of having an extra check that the input material is valid. (And that would require a check anyway against the range of valid material IDs, which this simple bool cannot do.)
There was a problem hiding this comment.
I think that's fine (but we should add a check in the GeneratorExecutor that the material is valid).
| // SPDX-License-Identifier: (Apache-2.0 OR MIT) | ||
| //---------------------------------------------------------------------------// | ||
| //! \file larceler/Convert.hh | ||
| //! \todo Update with #2223 |
There was a problem hiding this comment.
I will delete this now that it's merged in.
| */ | ||
| void LarStandaloneRunner::hit(SpanCelerHits hits) | ||
| { | ||
| CELER_LOG(debug) << "Processing " << hits.size() << " hits"; |
There was a problem hiding this comment.
Should probably make CELER_LOG_LOCAL? We also don't yet have a plan for managing thread-local state with global celeritas setup.
| Real3 larpos{convert_to_larsoft<LarsoftLen>(h.position[0]), | ||
| convert_to_larsoft<LarsoftLen>(h.position[1]), | ||
| convert_to_larsoft<LarsoftLen>(h.position[2])}; |
4 participants
This sets up detectors and a hit callback for LArSoft integration, and successfully records realistic BackTrackerRecord (BTR) results from the standalone test. It is not yet tested as part of a postprocessing art workflow (due to missing optical data in the DUNE inputs) and is known not to work as part of a full sim→ionandscint→fullsim workflow due to the use of an extant Geant4 physics setup for the EM stage.
GeantSetupoption to define detectors by name that get picked up during GeantGeo construction; for LArSoft this is set to{"PhotonDetector"}and propagated through the OpticalStandaloneInputfind_volume_instance_atfor vecgeom (@amandalund )LarStandaloneRunnerconstructionLarStandaloneRunner, and convert back to OBTR at end of eventI ran the test successfully against v10_14_01 and v10_20_01 (which both, I think, have the same data objects).