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

[10.2.X] add a veto DB update boolean in the SiPixelAli harvesting to decide to block the payload creation #23697

Merged
merged 2 commits into from Jul 2, 2018

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Jun 27, 2018

Greetings,
this PR solves two bugs in the SiPixelAli PCL alignment DQM monitoring, that were uncovered in the discussion of this e-mail thread:

  • the previous logic of the SiPixelAli DQM harvester forced to break from a while loop on the coordinates if there is a movement exceeding thresholds. This in turn entailed that if there was a movement exceeding the thresholds the DQM monitoring was crippled (all the other alignable parts were not plotted). I introduce a new boolean to decide whether to veto the upload, still not breaking the loop (so that all parts are plotted)
  • a bug in MillePedeDQMModule.cc causing the thresholds on several parameters for theta_Z alignable to be taken instead from the theta_Y.

Tested with

cmsDriver.py pedeStep --data --conditions 101X_dataRun2_Express_v8 --scenario pp -s ALCAHARVEST:SiPixelAli --dasquery='file dataset=/StreamExpress/Run2018B-PromptCalibProdSiPixelAli-Express-v1/ALCAPROMPT run=318622' -n -1

The situation before the fix:

image

becomes after the fixes:

image

N.B.: no change in behavior for the actual payload creation is intended here, the changes shall affect only the monitoring

@cmsbuild cmsbuild added this to the CMSSW_10_2_X milestone Jun 27, 2018
@mmusich
Copy link
Contributor Author

mmusich commented Jun 27, 2018

type bug-fix

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 27, 2018

@hroskes @connorpa you are interested in following this.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

Alignment/MillePedeAlignmentAlgorithm

@arunhep, @cerminar, @cmsbuild, @franzoni, @pohsun, @lpernie can you please review it and eventually sign? Thanks.
@mschrode, @mmusich, @tocheng, @tlampen 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

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 27, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28925/console Started: 2018/06/27 18:44

@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-23697/28925/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2899480
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2899288
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 128 log files, 14 edm output root files, 31 DQM output files

@arunhep
Copy link
Contributor

arunhep commented Jun 28, 2018

+1

@cmsbuild
Copy link
Contributor

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)

@mmusich
Copy link
Contributor Author

mmusich commented Jun 28, 2018

@arunhep is there still any potential interest in backporting this?

@arunhep
Copy link
Contributor

arunhep commented Jun 28, 2018

@mmusich i think it will be good to have the backport. @fabiocos is going to built 10_1_8 soon so we can have this bug fix in there.
Thanks for the prompt action.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 28, 2018

@arunhep, thanks for the reply. The backport is here: #23711

@fabiocos
Copy link
Contributor

fabiocos commented Jul 2, 2018

@mmusich @arunhep this looks as a genuine fix, so it is ok for me to enter in 10_2_X now. Anyway it is going to affect the express workflow, am I correct?

@mmusich
Copy link
Contributor Author

mmusich commented Jul 2, 2018

@fabiocos, it will affect the DQM output in the SiPixelAli ALCAPROMPT dataset which is produced during the AlCaHarvesting step of Express, so in a sense it is changing the express workflow, but it should not affect anything relevant for physics, if that's what you mean.

@fabiocos
Copy link
Contributor

fabiocos commented Jul 2, 2018

+1

@cmsbuild cmsbuild merged commit 60495a7 into cms-sw:master Jul 2, 2018
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.

None yet

5 participants