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.0.x] Update of SiStrip PI classes #21265

Merged
merged 7 commits into from Nov 20, 2017
Merged

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Nov 10, 2017

Features added:

  • Implement different levels of saturation for the TrackerMaps via class templates;
  • Add plot of integral gain distribution comparison;
  • Add 2D plots of gain ratios;
  • Correct nomenclature; rename everywhere "partition" to more appropriate "region";

Bug-fixes:

  • Fixed buggy logic in filling the TrackerMap of the maximum deviation of the ratio;
  • Fixed off-by-one bin filling in SiStripApvAbsolute* plots;

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21265/1933

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-21265/1933/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-21265/1933/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@mmusich
Copy link
Contributor Author

mmusich commented Nov 10, 2017

type bug-fix

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21265/1936

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CondCore/SiStripPlugins

@ggovi, @cmsbuild can you please review it and eventually sign? Thanks.
@ghellwig, @mmusich, @VinInn this is something you requested to watch as well.
@davidlange6, @slava77 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 Nov 10, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24361/console Started: 2017/11/10 19:38

@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-21265/24361/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2832699
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2832520
  • DQMHistoTests: Total skipped: 178
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.89000000005 KiB( 23 files compared)
  • Checked 111 log files, 8 edm output root files, 27 DQM output files

@ggovi
Copy link
Contributor

ggovi commented Nov 14, 2017

+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 (and backports should be raised in the release meeting by the corresponding L2)

@mmusich
Copy link
Contributor Author

mmusich commented Nov 20, 2017

Hello,
is there any problem with integrating this PR?
Thanks,

case SiStripPI::TID3s: return "TID D3 stereo";
case SiStripPI::END_OF_REGIONS : return "undefined";
default : return "should never be here";
case SiStripPI::TIB1r: return std::make_pair(1 ,"TIB L1 r-#varphi");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @mmusich - sorry for the delay - how do you plan to use this new index ? (I don't think its used below). The enum has an index already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @davidlange6, I need it to get the bin number to fill in e.g. https://github.com/cms-sw/cmssw/pull/21265/files#diff-0eaa5baaa2c31b79f1f2bbf7573a8eb3R1454.
I cannot use the indexing already associated to the enum because that one is used to couple it to internal indexing used in SiStripDetSummary

if( layer <= stereo[subDet] ) valueMap_[1000*(subDet+1)+layer*10+1] = initValues;

@davidlange6
Copy link
Contributor

davidlange6 commented Nov 20, 2017 via email

@davidlange6
Copy link
Contributor

davidlange6 commented Nov 20, 2017 via email

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

4 participants