reduce cases of LogWarnings with InvalidHelix category in Pixel/Strip ClusterShape compatibility checks #23209
Conversation
|
The code-checks are being triggered in jenkins. |
|
@cmsbuild please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23209/4669 |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @slava77 (Slava Krutelyov) for master. It involves the following packages: RecoPixelVertexing/PixelLowPtUtilities @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
slava77
changed the title
| if(!helix.isValid()) edm::LogWarning("InvalidHelix") << "PixelClusterShapeSeedComparitor helix is not valid, result is bad"; | ||
| if(!helix.isValid() //check still if it's a straight line, which are OK | ||
| && !helix.circle().isLine())//complain if it's not even a straight line | ||
| edm::LogWarning("InvalidHelix") << "PixelClusterShapeSeedComparitor helix is not valid, result is bad"; |
There was a problem hiding this comment.
Interesting.... is the efficiency improving?
There was a problem hiding this comment.
Actually wonder what
float xc = helix.circle().x0(), yc = helix.circle().y0();
would then do
There was a problem hiding this comment.
there is no change in physics in this PR, it just reduces the range of cases when the logWarning is issued.
The xc,yc are (0,0) for the case of a line.
There was a problem hiding this comment.
ok, we need to have a further look on how to use a straight line
There was a problem hiding this comment.
In the LogWarnings that I have investigated
this code is called by RecoTracker/TkSeedGenerator/plugins/SeedFromConsecutiveHitsTripletOnlyCreator.cc:53
there the code already apparently makes some assumptions about !helix.isValid() and already makes up a state at vertex for an invalid helix which simply points from the vertex to the outer hit (just as if it's a straight line)
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 differences looks related to the known issue |
InvalidHelix is one of the leading LogWarnings in current pp processing.
It turns out that a large fraction (all?) of the warnings corresponds to the reference helix considered "invalid" because it was reduced to a straight line. This is actually a valid input.
FastCircle::isLine method is added to make the simple test.
There is no change to the physics results of the algorithms. A LogWarning is simply not issued anymore in the case that appears to already have a correct behavior.