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

Mtd parameters update for D49 and D60 geometries (Bug fix) #33647

Merged
merged 2 commits into from May 7, 2021

Conversation

parbol
Copy link
Contributor

@parbol parbol commented May 6, 2021

PR description:

This PR fixes a bug introduced by #33340, affecting the D49 and D60 geometries. That PR reinterpreted the numbers in the BTL and ETL number vectors in the mtdParameters.xml file as interpad/and pad-to-border distances to define the dead areas around the pads, however the change was not made for the mtdParameters.xml being used by the D49 and D60 geometries. This PR updates also those numbers setting the distances as "0" to exactly reproduce the behaviour of the D49 and D60 geometries.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33647/22523

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

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

It involves the following packages:

Geometry/MTDCommonData

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @srimanob, @kpedro88 can you please review it and eventually sign? Thanks.
@fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor

fabiocos commented May 6, 2021

please test

minor effects expected

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0dc508/14914/summary.html
COMMIT: 74053ae
CMSSW: CMSSW_12_0_X_2021-05-06-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33647/14914/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: 5482 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2662646
  • DQMHistoTests: Total failures: 65774
  • DQMHistoTests: Total nulls: 3
  • DQMHistoTests: Total successes: 2596847
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@fabiocos
Copy link
Contributor

fabiocos commented May 6, 2021

@parbol changes as naively expected I would say. The numbers incorrectly used as microns describing a gap were all quite small.

@parbol
Copy link
Contributor Author

parbol commented May 6, 2021

@fabiocos Yes, I think this is the expected behaviour.

@cvuosalo
Copy link
Contributor

cvuosalo commented May 6, 2021

+1

@srimanob
Copy link
Contributor

srimanob commented May 6, 2021

+Upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

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

@qliphy
Copy link
Contributor

qliphy commented May 7, 2021

+1

@cmsbuild cmsbuild merged commit 0eb8897 into cms-sw:master May 7, 2021
@fabiocos
Copy link
Contributor

fabiocos commented May 7, 2021

This fix was already in #33598 , anyway this can be merged earlier. It would be useful for consistency to get it in 11_3_X in case someone wants to use D49, in order to avoid small but non properly defined differences with earlier versions.

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

6 participants