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

Validation/Geometry: Tracker/HGCal Material Budget 'Other' Category fix [Backport] #25143

Merged
merged 11 commits into from
Nov 28, 2018

Conversation

vargasa
Copy link
Contributor

@vargasa vargasa commented Nov 6, 2018

Backport of #25072

Please note that commits: e7ccffa, 8c61b57, and 84d9708 were not included as it is better not to touch the file structure of the release as twiki documentation may relay on it

For details please check:

vargasa and others added 11 commits November 6, 2018 16:37
In the past, the statement to save the histograms in a ROOT file
was written in the destructors of some classes, with the later use of
Smart Pointers the ROOT file was not being written.

endOfRun(), which should be called explicitely from MaterialBudgetAction
includes a call to TestHistoMgr::save().
Replacing std::cout for edm::Log* methods to clean up output.
…her'

	* It was crashing when testing for Extended2017Plan1 Geometry (Flange1_CFK)
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

A new Pull Request was created by @vargasa (Andres Vargas) for CMSSW_10_3_X.

It involves the following packages:

Validation/Geometry

@andrius-k, @Dr15Jones, @kmaeshima, @cvuosalo, @schneiml, @ianna, @mdhildreth, @cmsbuild, @jfernan2, @civanch can you please review it and eventually sign? Thanks.
@rovere 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

@andrius-k
Copy link

Hi @vargasa, the changes between this PR and the forward-port seem to be different. The number of commits also don't match. Could you please take a look at it?

@vargasa
Copy link
Contributor Author

vargasa commented Nov 6, 2018

@andrius-k as I pointed out in the description: Please note that commits: e7ccffa, 8c61b57, and 84d9708 were not included as it is better not to touch the file structure of the release as twiki documentation may relay on it

@andrius-k
Copy link

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31516/console Started: 2018/11/07 10:39

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3163099
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3162901
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@andrius-k
Copy link

Hi @vargasa, the back-port has 11 commits and the original PR has 7, out of which 3 were not back-ported. That means that only 4 commits in the back-port come from original PR. Where do the others come from? Maybe this is a back-port of few PRs?

@vargasa
Copy link
Contributor Author

vargasa commented Nov 7, 2018

Hi @andrius-k. This is because this PR was written on top of another backport #25139 which has not been merged yet (it is missing geometry and orp approval). There should be no problem with that. Maybe @cvuosalo could give us a hand approving #25139 if everything looks fine from his side

@andrius-k
Copy link

backport of #25139
@vargasa ah ok, I see.

@andrius-k
Copy link

+1

@fabiocos
Copy link
Contributor

fabiocos commented Nov 8, 2018

@vargasa @kpedro88 why is this backport needed in 10_3_X?

@ianna
Copy link
Contributor

ianna commented Nov 8, 2018

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 8, 2018

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_3_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_4_X is complete. 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)

@vargasa
Copy link
Contributor Author

vargasa commented Nov 8, 2018

@fabiocos Please check 24b5920#r31034588.

FYI @andrius-k

@kpedro88
Copy link
Contributor

kpedro88 commented Nov 8, 2018

I do not know why this needs a backport.

@vargasa
Copy link
Contributor Author

vargasa commented Nov 8, 2018

@kpedro88 this is needed. Again please check 24b5920#r31034588 Or am I missing something?

It was also suggested by @andrius-k #25072 (comment)

@kpedro88
Copy link
Contributor

kpedro88 commented Nov 8, 2018

@vargasa so this is needed to fix a bug in the tracker material budget analysis? That analysis definitely needs to proceed with 10_3_X rather than 10_4_X?

@vargasa
Copy link
Contributor Author

vargasa commented Nov 8, 2018

@vargasa so this is needed to fix a bug in the tracker material budget analysis?

Yes.

That analysis definitely needs to proceed with 10_3_X rather than 10_4_X?

It is up to you guys to include it. Just keep in mind that if someone uses Validation/Geometry/src/MaterialBudgetData.cc as it is in 10_3_X they will be getting wrong results

@vargasa
Copy link
Contributor Author

vargasa commented Nov 27, 2018

@fabiocos Could you please merge this as well? #25139 (comment) also applies here

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 126d155 into cms-sw:CMSSW_10_3_X Nov 28, 2018
@vargasa vargasa deleted the MatBdgOtherFix_103X branch December 7, 2018 15:02
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.

7 participants