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

fix(dRICH): CentralCKFTrajectories -> CentralCKFActsTrajectories #880

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

c-dilks
Copy link
Member

@c-dilks c-dilks commented Aug 30, 2023

Briefly, what does this PR introduce?

Fix dRICH track propagation by using CentralCKFActsTrajectories instead of CentralCKFTrajectories

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, should restore functionality to dRICH

@github-actions github-actions bot added the topic: PID Relates to PID reconstruction label Aug 30, 2023
c-dilks added a commit to eic/drich-dev that referenced this pull request Aug 30, 2023
c-dilks added a commit to eic/drich-dev that referenced this pull request Aug 30, 2023
@wdconinc
Copy link
Contributor

As long as there is nothing in the default output files that relies on the DRICH code, we are going to keep breaking the DRICH unintentionally because it is not tested for... So, you could (have) avoid(ed) this by adding DRICHAerogelTracks and DRICHGasTracks to the JEventProcessorPODIO.cc list, probably even as part of this PR.

@c-dilks
Copy link
Member Author

c-dilks commented Aug 30, 2023

As long as there is nothing in the default output files that relies on the DRICH code, we are going to keep breaking the DRICH unintentionally because it is not tested for... So, you could (have) avoid(ed) this by adding DRICHAerogelTracks and DRICHGasTracks to the JEventProcessorPODIO.cc list, probably even as part of this PR.

@wdconinc
Copy link
Contributor

#829 is on my list to review and merge this week...

@c-dilks
Copy link
Member Author

c-dilks commented Aug 30, 2023

https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/merge_requests/293 is also ready (I just realized I never marked it "ready"), but is only useful if someone routinely checks the plots. This particular track propagation issue just showed up as empty plots.

@wdconinc
Copy link
Contributor

only useful if someone routinely checks the plots

We have several benchmarks that fail if there are no entries when there are entries expected.

@c-dilks
Copy link
Member Author

c-dilks commented Aug 30, 2023

only useful if someone routinely checks the plots

We have several benchmarks that fail if there are no entries when there are entries expected.

We should add this feature, this MR is just a starting point. I keep trying to push @chchatte92 to improve the benchmarks, since he now does quite a lot more by hand :)

@chchatte92
Copy link
Member

only useful if someone routinely checks the plots

We have several benchmarks that fail if there are no entries when there are entries expected.

We should add this feature, this MR is just a starting point. I keep trying to push @chchatte92 to improve the benchmarks, since he now does quite a lot more by hand :)

Absolutely. Let's say, I will add at least these three-four features by next week to the benchmarks

  • Gaussian Fits for all the Residuals and theta distributions.
  • Apart from SPE Cherenkov angle as a function of phi, also SPE resolution.
  • Poisson Fit to the NPE distribution

Does it sound ok?

@c-dilks
Copy link
Member Author

c-dilks commented Aug 30, 2023

Testing this fix with benchmarks using eic/drich-dev#98 since it seems I never added them to the CI config.

And adding benchmarks to the CI config with https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/merge_requests/306

@c-dilks c-dilks changed the title test(dRICH): CentralCKFTrajectories -> CentralCKFActsTrajectories fix(dRICH): CentralCKFTrajectories -> CentralCKFActsTrajectories Aug 31, 2023
@c-dilks
Copy link
Member Author

c-dilks commented Aug 31, 2023

eic/drich-dev#103 blocks me from testing eta and momentum scans, but at least for single fixed momenta, dRICH PID appears to be recovered after this PR.

@c-dilks c-dilks marked this pull request as ready for review August 31, 2023 19:16
@c-dilks c-dilks requested a review from wdconinc August 31, 2023 19:16
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.

Yup.

@wdconinc wdconinc enabled auto-merge (squash) August 31, 2023 19:35
@wdconinc wdconinc merged commit 143e76b into main Aug 31, 2023
51 checks passed
@wdconinc wdconinc deleted the pr/drich-acts branch August 31, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: PID Relates to PID reconstruction
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants