-
Notifications
You must be signed in to change notification settings - Fork 82
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
Adding time-based cosmic track fit #198
Conversation
Hi @bonventre,
which require these tests: build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like important upgrades and the code is good, just a few small things to fix/review.
@@ -237,6 +237,12 @@ StrawResponse : { | |||
0.156641, 0.151196, 0.146546, 0.144069, 0.139858, 0.135838, 0.13319, | |||
0.132159, 0.130062, 0.123545, 0.120212 ] | |||
|
|||
useParameterizedDriftErrors : false | |||
parameterizedDriftBins : 25000 | |||
parameterizedDriftSigma : 1.39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these stable configuration parameters, or do we expect them to change due to (say) changing resolution or thresholds or...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are definitely calibrations that will change and be updated similarly to the rest of the StrawResponse parameters. I extract these parameters using the TrackerMCTune package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these parameters come from a proditions table eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we will have to plan out how tables will look and how they will get updated as we analyze the VST data.
@@ -31,16 +29,9 @@ namespace mu2e { | |||
TrkFitFlag _status; // status of processes used to create this seed | |||
// helixOK: ???, helixConverged: ???, circleInit: ???, Straight: ???, hitsOK: ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can safely ignore those bits (not set them). We could discuss if it's worthwhile to generalize the 'init' bits in this flag. hitsOK should have the same meaning here (enough hits to perform the fit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are still used by @sophiemiddleton so I'll leaving any changes here up to her
@FNALbuild run build test |
⌛ The following tests have been triggered for ref b932e55: build |
☀️ The tests passed at ref b932e55.
For more information, please check the job page here. |
@FNALbuild run build test |
⌛ The following tests have been triggered for ref 9e96991: build |
☀️ The tests passed at ref 9e96991.
For more information, please check the job page here. |
New modules:
Updated cosmic track algorithm:
Modified StrawResponse to help minuit convergence: