-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Updated BTLDetId format potentially breaking backward compatibility #47114
Comments
cms-bot internal usage |
A new Issue was created by @fabiocos. @Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
A possible way out we are considering is to introduce a new geometry scenario for BTL, v4, that is in fact a replica of v3, but for some minor naming variation needed to distinguish the two. The new numbering scheme (new detailed constructor) should be associated to BTL v4 scenario, while the old one remains associated to BTL v3, and the
We are testing this approach, to reintroduce later the update without breaking the backward compatibility in some cases. |
assign upgrade, geometry |
New categories assigned: upgrade,geometry @bsunanda,@civanch,@Dr15Jones,@kpedro88,@makortel,@mdhildreth,@Moanwar,@srimanob,@subirsarkar you have been requested to review this Pull request/Issue and eventually sign? Thanks |
The recent PR #46977 updates the BTL numbering scheme, and correspondingly the
BTLDetId
data format, in its expected final format, as the layout is now frozen. This implies that the meaning of bit fields changes, this is tracked in the new version of the format with a dedicated bit, distinguishing old and new, with a translation function that is applied in the constructors depending on the presence of this bit or not. The schema evolution of theBTLDetId
object is correctly managed in the IO rule.Until the explicit construction of
BTLDetId
is involved, everything works smoothly, and backward compatibility is ensured by the id translation made in the constructors, as tested by processing in a typical runTheMatrix workflow step-1 input made with the old format with code using the new format. Anyway when access is needed to a collection storing theBTLDetId
in its bareuint32_t
format, and no explicit conversion is made, this may cause failures as that identifier is not stored in the geometry, based on the new format, as described in #46977 (comment). A typical example is from the standaloneMtdValidation
codehttps://github.com/cms-sw/cmssw/blob/master/Validation/MtdValidation/test/mtdValidation_cfg.py#L78
that reruns the tracking rechit construction, at this point failing while accessing detids in the cluster collection and checking them against the
MTDGeometry
inhttps://github.com/cms-sw/cmssw/blob/master/RecoLocalFastTime/FTLRecProducers/plugins/MTDTrackingRecHitProducer.cc#L91
without making any explicit creation of specialised detid formats.
I am not sure any issue might be seen in the production of RelVal samples, as it seem not to create problems in IBs. But simple analysis examples like that above clearly are affected, and needs some protection. To what extent that may be acceptable, or what possible alternative solution could be envisaged, should be better agreed before producing potentially problematic builds.
The text was updated successfully, but these errors were encountered: