-
Notifications
You must be signed in to change notification settings - Fork 0
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
Candcloner bugfixes and follow up mitigation #102
Conversation
1. Loop over all available hits, until max-cand output slots are filled. 2. Make sure Kalman update is done on the main selected hit (not overlap). 3. Do not bump candidate chi2 when adding overlap hits.
I'm attaching TTbar and softQCD comparisons without cand. level DNN (in high pT triplet and pixelLess) blue is CKF, red is baseline without cand DNN and black is after the fix in cand cloner. *chi2 is also propagated from mkFit to the cand level DNN https://github.com/cms-sw/cmssw/blob/master/RecoTracker/MkFit/plugins/MkFitOutputConverter.cc#L693. |
|
…o the Kalman update plane in the barrel (can be controlled by KFO_Local_Cov)
I pushed the "local" covariance updates to this PR f8f6ada...eecfaba This effectively applies a restriction of the track state covariance to the plane where the Kalman update is made. The procedure is to rotate the space coordinates to have x orthogonal to the update plane (the same angle of rotation as the one used in the Kalman projection matrix), then zero-out the x component of the covariance and rotate back. The implementation in the code is after the explicit expansion and can be controlled with This affects only the barrel Kalman updates; in the endcap the update is already done properly in the xy plane. In the following, the impact of these commits can be seen between black and orange:
|
it turns out that while transferring from my debugging/testing setup in 12_4_0 to the mergeable in CMSSW version in 12_5_0_pre2, I missed a switch to enable the covariance matrix restriction on the backward fit. This is now pushed in 7f5d194 in this PR. |
as noted on Monday, I have pushed a bugfix in the propToR Jacobian f71fb49. Looking back, the effective flip of the sign happened in trackreco/mkFit#46 in trackreco/mkFit@a55225a#diff-12bbb3341241b082ec368e3bc565eddc0d144431034349ce475245bffd52ddebL453-R452 Effectively, only the first step of the 5-iteration propagation contributed to the Jacobian derivatives computation. After the fix, each iteration step contributes to the accumulation of affected derivatives. One side effect is that cov used to compute chi2 (small step propagation from the nominal radius) and cov used in update (large step propagation from the previous hit layer) was inconsistent. For one specific "friend" (pixelLess candidate with state params
After this fix the "friend" backward propagation has reasonable chi2 (max at 3.4 per hit), while before the fix some hits were going over chi2 of 200. Even though the change seems significant, the changes in physics are fairly small, as seen in the incremental comparison with 6-iter mkFit (with DNN on candidates disabled) and (blue is before the fix)
|
one more commit 4b7c60f ( fix matched layer counting in nHitsByTypeEncoded and nLayersByTypeEncoded). in one example of a candidate in the barrel with valid hits on layers This fix can technically affect the detachedTriplets, but there the effect seems almost invisible. Most of the visible change is in the pixelLess step and in particular in cases without attached pixel hits. Validation plots with candidate DNN disabled and using our 6-iter mkFit setup; comparing CKF (blue) with baseline as in CMSSW_12_4_0 (red), next-to-last commit (black), and the last commit (orange), without changes in the candidate filters: |
One more commit is added aad62ce, to account for dphi spread corresponding to the layer thickness. It is used in the I tried to apply this more generally, but could not find a positive effect:
Plots for the last commit are (comparing ckf blue, mkfit in the release red, before last commit in black, and this commit in orange): |
I have merged the commit from @mmasciov |
the last commits include:
|
Based on the validation plots http://uaf-10.t2.ucsd.edu/~legianni/check-clonerFIX2/ ,
So, based on the observations, I think that this PR can not go as is
extended
comparisons are needed to see more details