Implement passive material crossings in KinKal track extrapolation#1883
Implement passive material crossings in KinKal track extrapolation#1883RobMina wants to merge 6 commits into
Conversation
|
☀️ The build tests passed at ca33b1a.
N.B. These results were obtained from a build of this Pull Request at ca33b1a after being merged into the base branch at 1d2690a. For more information, please check the job page here. |
Issues found1. static const VEC3 nullfield(0.0,0.0,1.0e-3);Returning a fabricated 1e-3 axial field outside all maps avoids the 2. 3. Magic numbers / TODOs left as literals (low)
4. Stability caveat is a real blocker for enabling by default (medium) 5. Member-init reordering in |
|
📝 The HEAD of |
brownd1978
left a comment
There was a problem hiding this comment.
Hi Rob, this is great work, thanks. It will take me some time to go through the details. I have a few general comments/questions:
- Did you try using 'git mv' to rename the CRV extraplation function file? That generally keeps the commit history and line-by-line provenance
- The addition of ExtrapolateToR is logical. I wonder if that could be consolidated with ExtrapolateToZ by adding a closed cylinder surface with corresponding intersection function in KinKal.
- The Tracker perimeter is now being defined in several ways. Those should be consolidated, with values coming from GeometryService, along with a generalized toTrackerPerimeter function.
- Hard-coding a fixed field outside the range is fine for testing, but the underlying problem(s) needs to be addressed at its root.
|
Hi Dave, Thanks for the review comments. The ExtrapolateCRV file changed >50% of the lines, so git registered it as a remove+add rather than a rename. I updated the tracker perimeter limits by reading the values from GeometryService and dropping the unnecessary fcl parameters. ExtrapolateToZ is still needed because it is used for the the tracker front plane by extrapolateTracker and for the TSDA by extrapolateTSDA (two different z planes that do not match the logic of the tracker perimeter). Reverted KKBField.cc -- the edits there were not needed for this PR. |
brownd1978
left a comment
There was a problem hiding this comment.
Thanks for the updates Rob, this is moving in the right direction. I have few more questions:
- What is the code overlap between ExtrapolateCylinders and ExtrapolatePlanes? they look very similar. Could they be consolidated as template ExtrapolateSurface?
- It would be nice to consolidate toTrackerEnds and toTrackerPerimeter. That might be beyond the scope of this PR.
…terials (DS and concrete).
…-unused max-R configuration for ExtrapolateToZ.
…mon ExtrapolateSurface template.
e60e035 to
a760290
Compare
|
Thanks Dave! I made these updates:
|
Add DS and concrete crossings during KinKal track extrapolation to the CRV (material was defined in the earlier PR #1868). Passive x'ings can optionally be stored in the KalSeed (off by default).
NB: to run stably, this still requires setting bfcorr=false (the current default) for CentralHelix fit/extrapolation. Running bfcorr=true CentralHelix requires upstream stability improvements in KinKal.