Skip to content
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 pixel track efficiency codes for phase 1 detector #23160

Merged
merged 2 commits into from
May 18, 2018

Conversation

chenxvan
Copy link
Contributor

@chenxvan chenxvan commented May 7, 2018

Added offline cuts for all the pixel detector. Added fiducial cuts and backpropagation for Layer 1 detector.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

A new Pull Request was created by @chenxvan for master.

It involves the following packages:

DQM/SiPixelPhase1Track

The following packages do not have a category, yet:

DQM/SiPixelPhase1Track
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@cmsbuild can you please review it and eventually sign? Thanks.
@idebruyn, @threus, @fioriNTU, @hdelanno this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@chenxvan
Copy link
Contributor Author

chenxvan commented May 7, 2018

Submitted a PR for the category:
cms-sw/cms-bot#982

@smuzaffar
Copy link
Contributor

assign dqm

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

New categories assigned: dqm

@jfernan2,@vazzolini,@vanbesien,@kmaeshima,@dmitrijus you have been requested to review this Pull request/Issue and eventually sign? Thanks

@boudoul
Copy link
Contributor

boudoul commented May 9, 2018

Hi @chenxvan , please edit your title : "Updated code for phase 1 detector" could mean that you updated the local reco (for example, since I don't know which code you updated) for hcal (which is also a phase1 detector ...) , and for which purpose ? please be more explicit in the title , thank you.

@chenxvan chenxvan changed the title Updated code for phase 1 detector Updated pixel track efficiency codes for phase 1 detector May 9, 2018
@chenxvan
Copy link
Contributor Author

chenxvan commented May 9, 2018

Hi @boudoul, I update the title. Please let me know if you need more info. Thanks

@boudoul
Copy link
Contributor

boudoul commented May 11, 2018

dear @jfernan2, @vazzolini, @vanbesien, @kmaeshima, @dmitrijus , could you please consider this PR for review ?

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 14, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27928/console Started: 2018/05/14 10:28

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 17, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28002/console Started: 2018/05/17 10:52

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23160/28002/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2740553
  • DQMHistoTests: Total failures: 9833
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2730537
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@fabiocos
Copy link
Contributor

@chenxvan @fioriNTU please check in the link provided above the differences for 140.53, i.e.

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_10_2_X_2018-05-16-2300+23160/26539/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/

one should at least ensure that just adding this PR only there is no change there

@davidlange6
Copy link
Contributor

davidlange6 commented May 17, 2018 via email

@makortel
Copy link
Contributor

On the other hand the changes shown in @fabiocos's link do not seem to be among the "usual suspects" of #23105.

@fioriNTU
Copy link
Contributor

@fabiocos checking the diff link you sent it looks like the PV plots perfectly match on top of each other, but they are flagged as "failing comparison" please check here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_10_2_X_2018-05-16-2300+23160/26539/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/OfflinePV_Alignment.html

For what concern this PR, it doesn't touch any reconstruction module, it only change the way the Layer 1 efficiency for Pixels is computed in DQM, it neither changes the total number of bins in DQM histograms. I would expect only a slight increase in processing time, but nothing can happen outside the Phase1 DQM.

@makortel
Copy link
Contributor

The differences indeed look numerical-only. If they are produced by fits, then the underlying cause may be the same as discussed in #23105.

@fabiocos
Copy link
Contributor

@davidlange6 @makortel yes, this is why I was suspicious, normally the problem of non reproducibility is seen in the Tracking and HLT folders, in this case the pattern of discrepancies is different. I have checked on the same machine rerunning manually the wf 140.53 with and without only this PR merged. Indeed the observed difference looks an artefact of the comparison done by the bot as far as I can see, as I manage to get agreement for 140.53 with the bot compare_using_files.py sequence

  • summary of 59557 tests:
    o Failiures: 0.00% (0/59557)
    o Nulls: 0.00% (0/59557)
    o Successes: 100.00% (59557/59557)
    o Skipped: 0.00% (0/59557)
    o Missing objects: 0

so provided there is no further comment and DQM signs I would say this PR could move forward

@fabiocos
Copy link
Contributor

@dmitrijus @jfernan2 this PR was already signed by Dmitrijus and the change since then looks trivial, I merge it

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@boudoul
Copy link
Contributor

boudoul commented May 25, 2018

Hi @chenxvan ( the question is also for @fioriNTU and @arossi83 ) Maybe I missed it but I didn't see any backport of this PR in 10_1_X, should be not also considering backporting this ? Thanks.

@fioriNTU
Copy link
Contributor

@boudoul thanks for pointing this out, @chenxvan a backport to 10_1_X is needed, please provide it as soon as possible

@boudoul
Copy link
Contributor

boudoul commented May 29, 2018

Hi @chenxvan , could you please backport asap ?

@chenxvan
Copy link
Contributor Author

@boudoul @fioriNTU sorry I missed the message. I just backport to 10_1_X. Here is the PR: #23378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants