Remove an unneeded assignment for passcuts_hit in SiPixelPhase1TrackEfficiency

[perrotta]

PR description:

A dead assignment for the bool variable passcuts_hit was reported by the static aalyzer: fixed here (L223)

I profited to move outside a loop a check on the same variable, which would have prevented anyhow any action inside that loop (L552)

Still, I have doubts about the logic of the code that follows L439: there passcuts_hit is initialized to true before the loop that starts at L442: therefore, if set to false during any of the iterations, it will remain false till the end of the loop. I have the impression that it should have been initialized to true within every iteration of that loop. Coul the responsibles of the code (@cms-sw/dqm-l2 should know how to contact them) have a look at it?

PR validation:

It compiles

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35058/24937

• This PR adds an extra 16KB to repository

[cmsbuild]

A new Pull Request was created by @perrotta (Andrea Perrotta) for master.

It involves the following packages:

• DQM/SiPixelPhase1Track (dqm)

@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks.
@arossi83, @hdelanno, @sroychow, @fioriNTU, @jandrea, @idebruyn, @threus this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[perrotta]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1780ca/18110/summary.html
COMMIT: 918dcd6
CMSSW: CMSSW_12_1_X_2021-08-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35058/18110/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 39
• DQMHistoTests: Total histograms compared: 3000352
• DQMHistoTests: Total failures: 12
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3000318
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
• Checked 165 log files, 37 edm output root files, 39 DQM output files
• TriggerResults: no differences found

[jfernan2]

+1

[cmsbuild]

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[perrotta]

@jfernan2 , any comment about:
"Still, I have doubts about the logic of the code that follows L439: there passcuts_hit is initialized to true before the loop that starts at L442: therefore, if set to false during any of the iterations, it will remain false till the end of the loop. I have the impression that it should have been initialized to true within every iteration of that loop. Could the responsibles of the code (@cms-sw/dqm-l2 should know how to contact them) have a look at it?"

[jfernan2]

@mmusich @sroychow any comment for Andrea's question?

[mmusich]

assign trk-dpg

[mmusich]

hold

[cmsbuild]

New categories assigned: trk-dpg

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

[cmsbuild]

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[mmusich]

@tsusa is going to have a more in-depth analysis.
From a quick look, the part of the code affected is related only to BPix Layer 1 and the size of the vector which is being looped upon expTrajMeasurements

cmssw/DQM/SiPixelPhase1Track/plugins/SiPixelPhase1TrackEfficiency.cc

Line 443 in 209b393

for (uint p = 0; p < expTrajMeasurements.size(); p++) {

when running step3 with runTheMatrix.py -l 11634.0 -t 4 -j 8 is (almost) always 2, but out of the two trajectory crossings, both on same DetId, only one (the first) seems to be meaningful (the other one has row = 0 and col = 0), so at the end of the day the bug (if it's formally there) is not affecting anything excepted overlaps.
I don't quite understand yet why the

  expTrajMeasurements =
          theLayerMeasurements_->measurements(*pxbLayer1_, tsosPXB2, *trackerPropagator_, *chi2MeasurementEstimator_)

returns a vector of size 2.

[tsusa]

There are also cases with three expected measurements, two belonging to the overlapping modules and the third one with row = 0, col = 0. For those cases, passcuts_hit initialization is done at the wrong place. It did not however impact the results, due to a fiducial cut in lines 466-468. There are also other parts of the code that should be changed/fixed (e.g. lines 494-499).

[perrotta]

There are also cases with three expected measurements, two belonging to the overlapping modules and the third one with row = 0, col = 0. For those cases, passcuts_hit initialization is done at the wrong place. It did not however impact the results, due to a fiducial cut in lines 466-468. There are also other parts of the code that should be changed/fixed (e.g. lines 494-499).

Thank you @tsusa !

Then, a few further fixes are needed for the original code. What do you suggest?
A) Merging this trivial PR, and you can submit your fixes on top of it
B) Add yourself a commit with the fixes to this PR (having ticked "Allow edit by maintainers", this should be allowed: you can just try it)
C) Make yourself another complete PR, and close this one

Please let us know, so that we can proceed

[tsusa]

@perrotta I would propose option C

[perrotta]

@perrotta I would propose option C

FIne. Please go ahead.
I'll close this one as soon as your new PR will appear on github.

[perrotta]

@perrotta I would propose option C

@tsusa do you hVW any news, or plan for it?

[perrotta]

@tsusa please let us know if you have any update about the possible replacement for this PR.

[perrotta]

@tsusa @mmusich is there any news on this?

[mmusich]

unhold

[mmusich]

@tsusa

is there still anything holding this back?

[mmusich]

@qliphy

ping https://github.com/orgs/cms-sw/teams/tracking-pog-l2

This is not Tracking POG code

[qliphy]

@qliphy

ping https://github.com/orgs/cms-sw/teams/tracking-pog-l2

This is not Tracking POG code

Ok, now removed.

[tsusa]

We will x-check this week

[perrotta]

We will x-check this week

@tsusa is there any news here?

[mmusich]

-1

• superseded by Fix SiPixel Layer 1 efficiency calculation #37778

[perrotta]

-1

• superseded by Fix Layer 1 efficiency calculation #37778

Great! Thank you @tsusa and @mmusich
We can close this one, then