cms-sw
Broken == operator with const reco::TrackBaseRef&
The following operation returns false, even when both the .key() and the .id() are the same.
In other words, when using
return tk.first == vtk
instead of
return tk.first.key() == vtk.key() && tk.first.id() == vtk.id()
no match is found, even if the tracks are the same.
The same logic here (in the same file) works. The only obvious difference is the usage of the const TrackingParticleRef& data type instead of const reco::TrackBaseRef& on the right hand side of the equality.
A new Issue was created by @bfonta.
@Dr15Jones, @ftenchini, @makortel, @mandrenguyen, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.
cms-bot commands are listed here
New categories assigned: core,reconstruction,simulation
@civanch,@Dr15Jones,@jfernan2,@kpedro88,@makortel,@mandrenguyen,@mdhildreth,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks
The difference seems to be that TrackingParticleRef is edm::Ref whereas reco::TrackBaseRef is edm::RefToBase. The latter indeed has a more complicated definition of "equality"
https://github.com/cms-sw/cmssw/blob/8e6dacc20a2d33017661c5b5d109282bb5496c8a/DataFormats/Common/interface/BaseHolder.h#L44-L50
A case where the equality comparison could fail is if the other side would come from edm::View.
The edm::RefToBase infrastructure is complicated, and it has been effectively deprecated for ~14 years (since the introduction of edm::Ptr, see also https://github.com/cms-sw/cmssw/issues/42737), so I'd rather not touch it at this point. We might have a chance of converting all uses of RefToBase to Ptr next year (to be discussed in phase2 software days).
I do not know the code but suspect that comparison is not universal, so instead of implementing == operator it may be enough to check couple of data members. == operator may be deleted, if not - one has to control inheritance and understand cases where it is used.
Just to be clear, from simulation and core side, is it OK to change this to be:
return tk.first.key() == vtk.key() && tk.first.id() == vtk.id()
instead of
return tk.first == vtk
?
Indeed, for SIM this is fine if it gives the desired behavior for this particular application.
Just for the record, this is being fixed via https://github.com/cms-sw/cmssw/pull/49609/.
