Skip to content
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

Add TaggerHitFilter and no-cals detectors #1424

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Add TaggerHitFilter and no-cals detectors #1424

merged 3 commits into from
Sep 3, 2024

Conversation

bloodyyugo
Copy link
Contributor

This resolves issue 1423...as title says, add a filter on the tagger hits and adds detectors with no calorimeters.

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I have a bunch of cleaning up comments
  • The auxiliary auxtype="DetectorName" is the biggest "real" suggestion
  • I'd really prefer to not have the 4 GeV geometry if possible

Biasing/include/Biasing/TaggerHitFilter.h Show resolved Hide resolved
Biasing/src/Biasing/TaggerHitFilter.cxx Show resolved Hide resolved
Biasing/src/Biasing/TaggerHitFilter.cxx Outdated Show resolved Hide resolved
Biasing/src/Biasing/TaggerHitFilter.cxx Outdated Show resolved Hide resolved
Biasing/src/Biasing/TaggerHitFilter.cxx Outdated Show resolved Hide resolved
Biasing/src/Biasing/TaggerHitFilter.cxx Outdated Show resolved Hide resolved
Detectors/data/ldmx-det-v14-no-cals/constants.gdml Outdated Show resolved Hide resolved
Detectors/data/ldmx-det-v14-8gev-no-cals/detector.gdml Outdated Show resolved Hide resolved
Detectors/data/ldmx-det-v14-8gev-no-cals/detector.gdml Outdated Show resolved Hide resolved
Detectors/data/ldmx-det-v14-8gev-no-cals/detector.gdml Outdated Show resolved Hide resolved

void TaggerHitFilter::checkAbortEvent(G4Track* track) {
if ((layer_count_.size() < layers_hit_) ||
((layer_count_.count(10) == 0) && (layer_count_.count(20) == 0))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a general question based on this selection:

I think these hit constraints are tighter than what is currently in place for truth tracking and track reconstruction, where we just require a minimum number of hits (unless this is implicitly accounted for in the target extrapolation step in the CKFProcessor). If so, do we want to add something to track reconstruction to also check for hit placement close to the target? Or maybe just for truth tracks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question...in Omar's presentation, he suggested this requirement and it seems sensible. I think, currently, the strategies may require these layers be hit anyway, [https://github.com/user-attachments/files/16818852/acts-v19-v36-tagger-comparisons-4gev.pdf](see this plot included for v36 upgrade), though we will want to extend the strategies used,. The extrapolation doesn't require any sort of layers-hit-pattern as far as I know (and if it does, we should get rid of it).

That said, I personally haven't studied the 2-sensors-closest to target requirement in this context...I don't know how much it a) cuts down on the background we might see, and b) how much is speeds up generation. With the way the code is right now, I don't think we loose any efficiency because I'm pretty sure they are required anyway. Or at least they are always included in tracks we see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have these numbers as named constexpr so they dont look more like magic number too?

@bloodyyugo bloodyyugo self-assigned this Aug 30, 2024
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I squashed the many commits into one, also removed the 4GeV geometry, I'm happy with the current state now!

@tvami tvami merged commit 8697f60 into trunk Sep 3, 2024
2 checks passed
@tvami tvami deleted the iss1423 branch September 3, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter on the number of sim hits in the tagger and add detectors with just tracker & scintillator
4 participants