Add KalSeed selector to Ptr collection creation

[brownd1978]

This PR adds functionality to select KalSeeds to the MergeKalSeeds module, used in make TrkAna trees. The default behavior is unchanged. The selector is implemented as a tool, to allow multiple different kinds of tools to be designed for different purposes without having to change the core code.

[FNALbuild]

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

• RecoDataProducts
• TrkReco

which require these tests: build.

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

:hourglass: The following tests have been triggered for f88b33669b71642a8fe202f773bc4244cc624f4f: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

:sunny: The build tests passed at f88b33669b71642a8fe202f773bc4244cc624f4f.

Test	Result	Details
test with	:white_check_mark:	Command did not list any other PRs to include
merge	:white_check_mark:	Merged f88b33669b71642a8fe202f773bc4244cc624f4f at fc6b3b1e954528ac4f5f8b3f0ad965633dc79e64
build (prof)	:white_check_mark:	Log file. Build time: 11 min 15 sec
ceSimReco	:white_check_mark:	Log file.
g4test_03MT	:white_check_mark:	Log file.
transportOnly	:white_check_mark:	Log file.
POT	:white_check_mark:	Log file.
g4study	:white_check_mark:	Log file.
cosmicSimReco	:white_check_mark:	Log file.
cosmicOffSpill	:white_check_mark:	Log file.
ceSteps	:white_check_mark:	Log file.
ceDigi	:white_check_mark:	Log file.
muDauSteps	:white_check_mark:	Log file.
ceMix	:white_check_mark:	Log file.
rootOverlaps	:white_check_mark:	Log file.
g4surfaceCheck	:white_check_mark:	Log file.
FIXME, TODO	:white_check_mark:	TODO (0) FIXME (0) in 5 files
clang-tidy	:large_orange_diamond:	0 errors 207 warnings
whitespace check	:white_check_mark:	no whitespace errors found

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

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.

[kutschke]

@FNALbuild run build test

[FNALbuild]

:hourglass: The following tests have been triggered for 8fe3613ec510110762bef87894fb89b7ca223263: build (Build queue has 1 jobs)

[FNALbuild]

:sunny: The build tests passed at 8fe3613ec510110762bef87894fb89b7ca223263.

Test	Result	Details
test with	:white_check_mark:	Command did not list any other PRs to include
merge	:white_check_mark:	Merged 8fe3613ec510110762bef87894fb89b7ca223263 at 520269747a35ab2865eacacb4bcdc47b169e479b
build (prof)	:white_check_mark:	Log file. Build time: 21 min 14 sec
ceSimReco	:white_check_mark:	Log file.
g4test_03MT	:white_check_mark:	Log file.
transportOnly	:white_check_mark:	Log file.
POT	:white_check_mark:	Log file.
g4study	:white_check_mark:	Log file.
cosmicSimReco	:white_check_mark:	Log file.
cosmicOffSpill	:white_check_mark:	Log file.
ceSteps	:white_check_mark:	Log file.
ceDigi	:white_check_mark:	Log file.
muDauSteps	:white_check_mark:	Log file.
ceMix	:white_check_mark:	Log file.
rootOverlaps	:white_check_mark:	Log file.
g4surfaceCheck	:white_check_mark:	Log file.
FIXME, TODO	:white_check_mark:	TODO (0) FIXME (0) in 5 files
clang-tidy	:large_orange_diamond:	0 errors 207 warnings
whitespace check	:white_check_mark:	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 8fe3613ec510110762bef87894fb89b7ca223263 after being merged into the base branch at 520269747a35ab2865eacacb4bcdc47b169e479b.

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.

[kutschke]

@AndrewEdmonds11 a gentle reminder that this PR is on your to-review list. Thanks.

[FNALbuild]

:memo: The HEAD of main has changed to f7f21b09ceb931ff0a19cf8c87e9b74c8631067c. Tests are now out of date.

[kutschke]

@aFNALbuild run build test

[kutschke]

@FNALbuild run build test

ooops - typo on previous attempt

[FNALbuild]

:hourglass: The following tests have been triggered for 8fe3613ec510110762bef87894fb89b7ca223263: build (Build queue has 1 jobs)

[FNALbuild]

:sunny: The build tests passed at 8fe3613ec510110762bef87894fb89b7ca223263.

Test	Result	Details
test with	:white_check_mark:	Command did not list any other PRs to include
merge	:white_check_mark:	Merged 8fe3613ec510110762bef87894fb89b7ca223263 at f7f21b09ceb931ff0a19cf8c87e9b74c8631067c
build (prof)	:white_check_mark:	Log file. Build time: 10 min 54 sec
ceSimReco	:white_check_mark:	Log file.
g4test_03MT	:white_check_mark:	Log file.
transportOnly	:white_check_mark:	Log file.
POT	:white_check_mark:	Log file.
g4study	:white_check_mark:	Log file.
cosmicSimReco	:white_check_mark:	Log file.
cosmicOffSpill	:white_check_mark:	Log file.
ceSteps	:white_check_mark:	Log file.
ceDigi	:white_check_mark:	Log file.
muDauSteps	:white_check_mark:	Log file.
ceMix	:white_check_mark:	Log file.
rootOverlaps	:white_check_mark:	Log file.
g4surfaceCheck	:white_check_mark:	Log file.
FIXME, TODO	:white_check_mark:	TODO (0) FIXME (0) in 5 files
clang-tidy	:large_orange_diamond:	0 errors 207 warnings
whitespace check	:white_check_mark:	no whitespace errors found

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

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]

thanks for the review Andy

On Mon, Jun 10, 2024 at 8:00 AM Andrew Edmonds @.***> wrote:

@.**** approved this pull request.

Thanks, Dave. This looks like a really useful feature. I have just a couple of comments about things that were unclear to me. I don't think either should prevent this from getting merged

In TrkReco/src/MergeKalSeeds_module.cc https://github.com/Mu2e/Offline/pull/1282#discussion_r1633366361:

@@ -53,7 +65,18 @@ namespace mu2e { if(ksch.isValid()){ auto const& ksc = *ksch; if(debug_ > 2) std::cout << seedcolltag << " Has " << ksc.size() << " KalSeeds" << std::endl;

• for(size_t ikseed = 0; ikseed <ksc.size(); ++ikseed){ mkseeds->emplace_back(ksch,ikseed); }
• for(size_t ikseed = 0; ikseed <ksc.size(); ++ikseed){
• auto const& kseed = ksc[ikseed];
• if( (!selector_) ) {
• mkseeds->emplace_back(ksch,ikseed);
• } else if ( selector_->select(kseed)){
• if(!selbest_ || mkseeds->size() == 0)

What's the motivation for having both a selbest boolean and a selector? Doesn't specifying a selector in the configuration imply that we want to use it?

There are 2 logically separate cases: the user might want to select all KalSeeds in an event which pass the selector, or they might want to select at most 1 KalSeed per event that passes the selector and is the 'best' one when compared to all the others that pass the selector. Since the selector is an optional parameter, its existence defines the first case. For the 2nd a separate bool is needed.

---

In TrkReco/src/SimpleKalSeedSelector_tool.cc https://github.com/Mu2e/Offline/pull/1282#discussion_r1633401838:

• double nhitfrac = 2*(test.nHits() - current.nHits())/(test.nHits() + current.nHits());
• if(nhitfrac > minsignhit_){
• // difference is significant

It's unclear to me what this is checking - it looks like its more than just whether one track has more hits than the other? A comment in the code to clarify would be good

This is described in the comment for the configuration parameter: fhicl::Atom minsignhit{Name("MinDeltaNHitFraction"), Comment("Minimum difference in the fractional number of hits to consider significant")}; This has the advantage over a source code comment in that it gets printed if the user types 'mu2e --print-description SimpleKalSeedSelector'

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1282#pullrequestreview-2107996411, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH57ZDJZXTOHPWT4PB6ALZGW5SNAVCNFSM6AAAAABI67DFD2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBXHE4TMNBRGE . You are receiving this because you were mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

[kutschke]

There are 2 logically separate cases: the user might want to select all KalSeeds in an event which pass the selector, or they might want to select at most 1 KalSeed per event that passes the selector and is the 'best' one when compared to all the others that pass the selector. Since the selector is an optional parameter, its existence defines the first case. For the 2nd a separate bool is needed.

A future-proofing question. Do you anticipate cases for which you might wish to select more than 1 and fewer than all that pass the selector? If you anticipate development along those lines, it might be wise to change selbest from a bool to an enum .

[brownd1978]

lets deal with that when and if it happens

On Mon, Jun 10, 2024 at 8:43 AM Rob Kutschke @.***> wrote:

There are 2 logically separate cases: the user might want to select all KalSeeds in an event which pass the selector, or they might want to select at most 1 KalSeed per event that passes the selector and is the 'best' one when compared to all the others that pass the selector. Since the selector is an optional parameter, its existence defines the first case. For the 2nd a separate bool is needed.

A future-proofing question. Do you anticipate cases for which you might wish to select more than 1 and fewer than all that pass the selector? If you anticipate development along those lines, it might be wise to change selbest from a bool to an enum .

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1282#issuecomment-2158702294, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH57ZAJ6OQLFO5FQHDIZTZGXCRTAVCNFSM6AAAAABI67DFD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYG4YDEMRZGQ . You are receiving this because you were mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

[kutschke]

As targets of opportunity, I would like to move us towards a style where:

https://github.com/Mu2e/Offline/blob/f7f21b09ceb931ff0a19cf8c87e9b74c8631067c/TrkReco/src/MergeKalSeeds_module.cc#L49

is written as: auto mkseeds(std::make_unique<KalSeedPtrCollection>());

I did this from memory so there might be a typo. The goal is to get rid of uses of new for which there is another solution. Then we scrub for inappropirate uses of new.

[kutschke]

lets deal with that when and if it happens

Ok - I just wanted to be sure that we had asked the question because it may require intrusive changes.

[kutschke]

There was a formatting error in my previous comment that ate the angle brackets. I edited the original