First pass at extracting digis from tracks #1343

edcallaghan · 2024-09-24T18:41:53Z

This PR implements a modules which deep-copies StrawDigis and StrawDigiWaveforms which are associated with a reconstructed track via KalSeed into new *Collections. ~~This PR is marked as a draft for now; more detail will be added here before it's necessary to initiate review.~~

New module:

TrackDigiExtractor: Given a KalSeedCollection, produces a StrawDigiCollection and StrawDigiADCWaveformCollection which contain data products pointed to by KalSeeds in the specified collection. Future development implement include data products from the calorimeter.

…ks, and implementation for tracker

FNALbuild · 2024-09-24T18:42:18Z

Hi @edcallaghan,
You have proposed changes to files in these packages:

Blinding

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for c086bf4: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild · 2024-09-24T18:58:54Z

☀️ The build tests passed at c086bf4.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `c086bf4` at `38f72f8`
build (prof)	✅	Log file. Build time: 08 min 48 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	🔶	0 errors 103 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at c086bf4 after being merged into the base branch at 38f72f8.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

brownd1978

I wonder if it makes sense to combine this module with the time-shifting module, as I suspect both will end up needing to reference a track, and both otherwise produce and consume the same products. It seem more efficient to do this in one pass.

FNALbuild · 2024-10-04T19:40:35Z

📝 The HEAD of main has changed to 5694fbe. Tests are now out of date.

…gi_extraction

edcallaghan · 2024-12-09T23:48:59Z

@FNALbuild run build test

FNALbuild · 2024-12-09T23:49:24Z

⌛ The following tests have been triggered for c086bf4: build (Build queue is empty)

FNALbuild · 2024-12-09T23:58:04Z

☀️ The build tests passed at c086bf4.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `c086bf4` at `11e1039`
build (prof)	✅	Log file. Build time: 04 min 19 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	🔶	0 errors 103 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at c086bf4 after being merged into the base branch at 11e1039.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

edcallaghan · 2024-12-10T00:02:57Z

I wonder if it makes sense to combine this module with the time-shifting module, as I suspect both will end up needing to reference a track, and both otherwise produce and consume the same products. It seem more efficient to do this in one pass.

@brownd I envision the two as needing to be distinct via the following reasoning:
i) The offspill job needs to write an output stream containing the digis which may be used for blinding. To minimize the size on disk, that stream would only consist of digis taken from this module.
ii) The onspill job performs the actual sculpting, which in this case (time) may depend on the onspill run configuration (we could change the digitization window, or something like that).

When you propose that this part of the chain would reference the track, I assume you mean to use some fit t0 as the reference time for the track. That makes sense, but we would use the time relevant for the fit after mixing these digis with onspill pileup, not the time of the relatively clean offspill track, so there's no need to propagate the latter from the offspill job.

brownd1978 · 2024-12-12T04:45:18Z

@FNALbuild run build test

FNALbuild · 2024-12-12T04:45:41Z

⌛ The following tests have been triggered for fefea3d: build (Build queue has 1 jobs)

FNALbuild · 2024-12-12T04:54:23Z

☀️ The build tests passed at fefea3d.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `fefea3d` at `e3e7160`
build (prof)	✅	Log file. Build time: 04 min 19 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	🔶	0 errors 103 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at fefea3d after being merged into the base branch at e3e7160.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

brownd1978 · 2024-12-12T05:32:50Z

Blinding/src/TrackDigiExtractor_module.cc

+    for (const auto& kalseed: *kalseeds){
+      // tracker
+      auto seeds = kalseed.hits();
+      for (const auto& seed: seeds){


Its not clear to me we want to keep the inactive hits in this use case.

The only place I can identify this being set is in the TrkStrawHitSeed constructor, where it propagates from a WireHitState --- it looks like this is still a fit-level construct (e.g., it contains the decision on left/right ambiguity), so is contextual to the fit. If that's the case, then I think we'd want to keep the inactive hits, as they could become active when fit in the presence of pileup. Is that a reasonable take?

Or, if I'm wrong about the above, then we have to ask whether only propagating active hits risks a break in blindness. Since these are overlaid onto pileup which could contribute hits to the fit but end up inactive, the answer may be no, but it's worth mentioning.

In OffSpill cosmic track events, the inactive hits will mostly be poorly measured hits created by the primary particle, so you want to keep them. However in a mixed event, many/most of the inactive hits will be from pileup, so you would not want to keep them if you planned to re-mix the extracted digis.
I think the current code covers 90% of the foreseeable use cases, lets go with this for now. If in future we want to use this code to extract digis from events with pileup we can extend it to include a hit selection algorithm.

Ok, that all makes sense.

brownd1978 · 2024-12-12T05:34:53Z

Blinding/src/TrackDigiExtractor_module.cc

+    auto strawadcss = event.getValidHandle<StrawDigiADCWaveformCollection>(_strawdigi_tag);
+
+    // sanity checks
+    if (strawhits->size() != strawdigis->size()){


This test relies on this module's config being consistent with what was used when fitting the tracks.

A way to ensure consistency is to use the KKTrack instead of KalSeed; KKTrack can have Ptrs back to underlying collections, but it cannot be persisted, due to payload that includes std::Ptrs. We can discuss if that makes sense in this context.

Hmmm, ok. The current implementation does depend on this check passing, as e.g. StrawHits and StrawDigis do not contain any ptrs to each other. To be sure that both for a given hit are included, we have to assume that the collections in which they are contained are aligned (and the best check we have for that is same-length).

There is a long-term issue that we need to improve the robustness of the digi->hit->trkhit indexing using ProductPtr, which didn't exist when the original codes were written. That's beyond the scope of this PR. We can deal with this code when we address that issue in general.

Sounds good to me.

brownd1978 · 2024-12-12T05:37:44Z

Blinding/src/TrackDigiExtractor_module.cc

+    public:
+      struct Config{
+        // track
+        fhicl::Atom<art::InputTag> kalseed_tag{


Is it conceivable this module will need to coherently process multiple KalSeed collections (ie multiple particle hypotheses)?

It is conceivable, but IMO unlikely for the conversion-blinding use case where we really only want electron-like tracks. Maybe we'll determine that selecting another stream is useful for some book-keeping purpose, but I doubt that we'd want to collate that with the electron-likes at this stage.

first pass at module to extract digis which participate in given trac…

c086bf4

…ks, and implementation for tracker

FNALbuild added the build running label Sep 24, 2024

FNALbuild added build finished and removed build running labels Sep 24, 2024

edcallaghan marked this pull request as draft September 24, 2024 19:06

brownd1978 reviewed Oct 3, 2024

View reviewed changes

FNALbuild added build pending and removed build finished labels Oct 4, 2024

Merge branch 'main' of github.com:Mu2e/Offline into blinding_track_di…

b3cab9f

…gi_extraction

FNALbuild added build running and removed build pending labels Dec 9, 2024

edcallaghan marked this pull request as ready for review December 9, 2024 23:50

FNALbuild added build finished and removed build running labels Dec 9, 2024

brownd1978 approved these changes Dec 11, 2024

View reviewed changes

resolve scons conflicts

fefea3d

FNALbuild added build pending and removed build finished labels Dec 11, 2024

FNALbuild added build running and removed build pending labels Dec 12, 2024

FNALbuild added build finished and removed build running labels Dec 12, 2024

brownd1978 reviewed Dec 12, 2024

View reviewed changes

brownd1978 merged commit 3398e30 into Mu2e:main Dec 12, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at extracting digis from tracks #1343

First pass at extracting digis from tracks #1343

edcallaghan commented Sep 24, 2024 •

edited

Loading

FNALbuild commented Sep 24, 2024

FNALbuild commented Sep 24, 2024

brownd1978 left a comment

FNALbuild commented Oct 4, 2024

edcallaghan commented Dec 9, 2024

FNALbuild commented Dec 9, 2024

FNALbuild commented Dec 9, 2024

edcallaghan commented Dec 10, 2024

brownd1978 commented Dec 12, 2024

FNALbuild commented Dec 12, 2024

FNALbuild commented Dec 12, 2024

brownd1978 Dec 12, 2024

edcallaghan Dec 12, 2024

brownd1978 Dec 12, 2024

edcallaghan Dec 12, 2024

brownd1978 Dec 12, 2024

edcallaghan Dec 12, 2024

brownd1978 Dec 12, 2024

edcallaghan Dec 12, 2024

brownd1978 Dec 12, 2024

edcallaghan Dec 12, 2024

First pass at extracting digis from tracks #1343

First pass at extracting digis from tracks #1343

Conversation

edcallaghan commented Sep 24, 2024 • edited Loading

FNALbuild commented Sep 24, 2024

FNALbuild commented Sep 24, 2024

brownd1978 left a comment

Choose a reason for hiding this comment

FNALbuild commented Oct 4, 2024

edcallaghan commented Dec 9, 2024

FNALbuild commented Dec 9, 2024

FNALbuild commented Dec 9, 2024

edcallaghan commented Dec 10, 2024

brownd1978 commented Dec 12, 2024

FNALbuild commented Dec 12, 2024

FNALbuild commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edcallaghan commented Sep 24, 2024 •

edited

Loading