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

tracking: use Seeded tracks by default, rename previous default to TruthSeeded #1605

Merged
merged 2 commits into from
Sep 8, 2024

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Aug 27, 2024

Briefly, what does this PR introduce?

As we agreed during the collaboration meeting, the real seeding workflow is mature and should become the default. This changes Seeded collections to not include that prefix, and prefixless collections are moved to TruthSeeding (name is long but not supposed to be used by general users). As a side effect, Jet finding and ReconstructedParticles and DRICH IRT-based PID will use real seeding.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

Yes

@github-actions github-actions bot added topic: PID Relates to PID reconstruction topic: barrel labels Aug 27, 2024
Copy link

github-actions bot commented Aug 27, 2024

@veprbl veprbl added this to the 24.09.0 milestone Aug 28, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
39.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Changes make sense. I did not verify that this is the complete set of changes required. I assume you've grepped for other instances.

@veprbl veprbl added this pull request to the merge queue Sep 8, 2024
Merged via the queue into main with commit e787018 Sep 8, 2024
87 of 88 checks passed
@veprbl veprbl deleted the pr/seeded_default branch September 8, 2024 15:20
@michael-pitt michael-pitt added the topic: far-forward Far forward reconstruction label Sep 29, 2024
@michael-pitt
Copy link
Contributor

Hi @veprbl ,

I'm not sure if I need to open an issue or if I can report it here. It seems that this RP also impacted the reconstruction of one of the far-forward sub-systems - the B0 Tracker. In the past we were using the ReconstructedChargedParticles container to get the B0 tracks. Since this RP, the B0 tracks moved to ReconstructedTruthSeededChargedParticles container. This might be OK since the b0 tracks are also using truth seeding, but I see that this container is called from src/detectors/DRICH/DRICH.cc#L211, which is not related to Far-Forward detectors. It wasn't easy to find out what had happened in this case.

I'm also motived some naming mismatch in src/global/tracking/tracking.cc:
L162 CentralCKFTrajectories -> CentralCKFActsTrajectoriesUnfiltered
L95 CentralCKFTruthSeededTrajectories->CentralCKFTruthSeededActsTrajectoriesUnfiltered

Will you confirm that the B0 tracks should be retrieved from ReconstructedTruthSeededChargedParticles container?

Thanks,
Michael

@veprbl
Copy link
Member Author

veprbl commented Sep 30, 2024

I'm not sure if I need to open an issue or if I can report it here. It seems that this RP also impacted the reconstruction of one of the far-forward sub-systems - the B0 Tracker. In the past we were using the ReconstructedChargedParticles container to get the B0 tracks. Since this RP, the B0 tracks moved to ReconstructedTruthSeededChargedParticles container. This might be OK since the b0 tracks are also using truth seeding, but I see that this container is called from src/detectors/DRICH/DRICH.cc#L211, which is not related to Far-Forward detectors. It wasn't easy to find out what had happened in this case.

Will you confirm that the B0 tracks should be retrieved from ReconstructedTruthSeededChargedParticles container?

Two things here: One is that realistic seeding does not work for B0 tracking. We need to have it implemented. Until we do that, I think it's for the best that we are reminded that truth seeding is being used by having to access the ReconstructedTruthSeededChargedParticles. Truth seeding can produce quite misleading performances. Another thing is about data processing for ReconstructedParticles passing though PID factories, that is needed to provide PID LUTs. The reconstructed particles are not just about tracking, this is a holistic object, even more detectors will be used in the future.

I'm also motived some naming mismatch in src/global/tracking/tracking.cc: L162 CentralCKFTrajectories -> CentralCKFActsTrajectoriesUnfiltered L95 CentralCKFTruthSeededTrajectories->CentralCKFTruthSeededActsTrajectoriesUnfiltered

This refers to the two previous PRs: #1470 #1383
The CentralCKF*Acts* tags are Acts structures that are not PODIO and can't be e.g. directly written to an edm4eic.root file. We employ conversion from Acts to PODIO for now, but in the future the goal is to have algorithms to exchange PODIO structures only.

@michael-pitt
Copy link
Contributor

OK, Thanks! (adding @ajentsch in case someone in FF group will have a similar question),

Indeed, I see that the central tracks are included in both ReconstructedTruthSeededChargedParticles and ReconstructedChargedParticles while B0 tracks are only in the ReconstructedTruthSeededChargedParticles container.

So, I will do the following:
Central tracks retrieved from ReconstructedChargedParticles
B0 tracks are retrieved from ReconstructedTruthSeededChargedParticles after applying an eta cut of 4.2 (trk_eta>4.2 in the hadron beam coordinate system).

@simonge
Copy link
Contributor

simonge commented Sep 30, 2024

Other than for simplicity is there a reason the B0 tracks are mixed into the same raw hit collection as the central tracker? Would it not be more useful to keep them separate until they are needed(?) by the vertex finder for timing. The same acts algorithms would be used with the same configuration, or something specifically tuned to the B0, without attempting to use B0 hits in central detector tracks and vice versa. This would allow a different approach to setting the PID or at least making a PID assumption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: barrel topic: far-forward Far forward reconstruction topic: PID Relates to PID reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants