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: disable momentum in reco/truth particle matching #496

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Feb 16, 2023

Briefly, what does this PR introduce?

This effectively disables the momentum in reconstructed vs truth particle matching, and increases the phi tolerance from 0.03 to 0.1 rad. Closes #450.

What kind of change does this PR introduce?

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 @bspage912

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

No.

Does this PR change default behavior?

Yes, less strict matching.

This effectively disables the momentum in reconstructed vs truth particle matching, and increases the phi tolerance from 0.03 to 0.1 rad.
@wdconinc wdconinc force-pushed the 450-reconstructedchargedparticlesassociations-with-too-often-missing-associations branch from bef0e64 to cd3b0b5 Compare February 16, 2023 21:39
@wdconinc
Copy link
Contributor Author

@bspage912 is this something you can easily test?

@bspage912
Copy link
Contributor

Hey Wouter, in principle this should be easy to test, just run one MC file through EICrecon with the new algo. In practice, it may take a bit as I will have to go back and remind myself how to run things. This change is so simple, I can't imagine any unexpected behaviour ...

@wdconinc wdconinc enabled auto-merge February 17, 2023 04:40
@wdconinc
Copy link
Contributor Author

@bspage912 Can you approve this here, just based on the changes and not testing if you agree with the changes? I can then tag a release and submit some jobs for more detailed testing.

Copy link
Contributor

@bspage912 bspage912 left a comment

Choose a reason for hiding this comment

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

While I have not explicitly tested this branch, I did emulate the matching algorithm in my offline tests without the relative momentum condition and with the phi tolerance changed to 0.1 and saw improved behaviour. This leads me to believe the changes in this branch should be fine to merge in.

@wdconinc wdconinc merged commit 87ed89a into main Feb 17, 2023
@wdconinc wdconinc deleted the 450-reconstructedchargedparticlesassociations-with-too-often-missing-associations branch February 17, 2023 06:58
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.

ReconstructedChargedParticlesAssociations with too-often missing associations
2 participants