PackedCandidate access to dzError or dxyError can lead to an exception on phase-1

slava77 commented 7 years ago

The problem was first observed in https://github.com/cms-sw/cmssw/pull/19025#issuecomment-306699120

Some packed candidates never have their covariance set to non-default value (no call to ::setTrackProperties by design of the packed candidate or lost-track producers). In phase-1 workflows this means that some candidates in the same collection have covarianceVersion set to 1 and some (small fraction) have it left at the default 0. Consequently, access to e.g. dxyError or similar method that requires covariance unpacking will fail with an exception and the job will terminate.

There is a workaround for user code to first check "hasTrackDetails()".

Possible resolutions:

force users to make a check hasTrackDetails or even explicitly check covarianceVersion consistency before accessing the dxyError and such [BAD]
embed hasTrackDetails() check in the PackedCandidate::unpackCovariance (looks the easiest and is perhaps most complete)
update producers to set a consistent covariance version for all candidates written out

@arizzi @gpetruc

cmsbuild commented 7 years ago

A new Issue was created by @slava77 Slava Krutelyov.

@davidlange6, @Dr15Jones, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

slava77 commented 7 years ago

assign reconstruction

cmsbuild commented 7 years ago

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

arizzi commented 7 years ago

@slava77 I'm not sure I understand what the proposal is... what should we return when people access something we do not store? I think the exception makes sense unless we want to return the mean values for those tracks, in which case the right thing to do is to define a schema for that and use it as default for all tracks with no track details stored (please note that because we do not store nhits for those tracks it will be a very rough approximation)

slava77 commented 7 years ago

On 6/9/17 8:06 AM, arizzi wrote:

@slava77 https://github.com/slava77 I'm not sure I understand what the proposal is...

the primary part of the proposal is not to have an exception on something as visibly basic as dzError

Another option is to improve the error message to at least provide a faster solution for the user code.

arizzi commented 7 years ago

the error message can be improved, but I'd rather keep the exception than returning random values... unless we want to go for the meanvalue (but again people should know that they are crappy values and I'm not sure how to pass the message)

slava77 commented 7 years ago

based on discussion, this is in a "will not fix" with arguments that the choice is by design closing without a +1.

slava77 commented 7 years ago

actually closing this issue now

cms-sw / cmssw

PackedCandidate access to dzError or dxyError can lead to an exception on phase-1 #19151