Run2-hcx155 Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon #20523

bsunanda · 2017-09-14T21:07:45Z

There is only one geometry for Hcal for all components (HB,HE,HF,HO) -> HcalGeometry

cmsbuild · 2017-09-14T21:08:03Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-14T21:10:50Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20523/709

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-20523/709/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20523/709/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

davidlange6 · 2017-09-14T22:42:50Z

please test

davidlange6 · 2017-09-15T00:42:49Z

please test

davidlange6 · 2017-09-15T02:42:49Z

please test

davidlange6 · 2017-09-15T04:42:49Z

please test

davidlange6 · 2017-09-15T06:42:49Z

please test

perrotta · 2017-09-15T10:49:47Z

@bsunanda please apply the fixes spotted by the code-cheks

cmsbuild · 2017-09-15T12:46:29Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-15T12:49:48Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20523/728

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-20523/728/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20523/728/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

cmsbuild · 2017-09-15T12:55:23Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-15T12:59:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20523/730

cmsbuild · 2017-09-15T12:59:31Z

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

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

bsunanda · 2017-09-15T13:07:54Z

@cmsbuild Please test

cmsbuild · 2017-09-15T13:08:12Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23007/console Started: 2017/09/15 18:08

cmsbuild · 2017-09-15T17:13:41Z

+1
Tested at: eaa68e6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20523/23007/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
9f20abe
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20523/23007/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20523/23007/git-merge-result

cmsbuild · 2017-09-15T17:13:44Z

Comparison job queued.

cmsbuild · 2017-09-15T20:49:28Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2646869
DQMHistoTests: Total failures: 207
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2646473
DQMHistoTests: Total skipped: 189
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 14 edm output root files, 26 DQM output files

slava77 · 2017-09-18T17:24:56Z

RecoTauTag/RecoTau/plugins/TauDiscriminationAgainstCaloMuon.cc

@@ -280,8 +270,7 @@ double TauDiscriminationAgainstCaloMuon<TauType, TauDiscriminator>::discriminate

  const CaloSubdetectorGeometry* ebGeometry = caloGeometry_->getSubdetectorGeometry(DetId::Ecal, EcalBarrel);
  const CaloSubdetectorGeometry* eeGeometry = caloGeometry_->getSubdetectorGeometry(DetId::Ecal, EcalEndcap);
-  const CaloSubdetectorGeometry* hbGeometry = caloGeometry_->getSubdetectorGeometry(DetId::Hcal, HcalBarrel);
-  const CaloSubdetectorGeometry* heGeometry = caloGeometry_->getSubdetectorGeometry(DetId::Hcal, HcalEndcap);
+  const HcalGeometry* hcGeometry = (HcalGeometry*)(caloGeometry_->getSubdetectorGeometry(DetId::Hcal, HcalBarrel));


wouldn't it make more sense to change the interface of CaloGeometry to add getSubdetectorGeometry(det) so that we do not have this counter-intuitive code which semantically looks like HcalBarrel subdetector is selected, while in reality it's all of Hcal.

i agree with @slava77 comment - this is a very confusing (and hacky) function call.. how can this be cleaned up?

I shall think about it. It is not a very easy solution - there are detectors which has one geometry for all sub detectors while there are detectors which have different geometry for each sub detector. I shall try to see if something can be done in near future. But that has to be a separate PR and this PR is simply a bug fix

it would seem a simple solution is to add a getDetectorGeometry and implement getSubdetectorGeometry only when relevant (throwing an exception otherwise). That way you don't have to find these "bugs" (rather, code that didn't follow an interface change that didn't change the interface itself) one by one

David EcalBarrelGeometry, HcalGeometry, .. all are derived from CaloSubDetectorGeometry and depending on the detector/subdetector the functionalities are somewhat different. I do not think what you are suggesting makes sense

slava77 · 2017-09-18T17:33:44Z

+1

for #20523 eaa68e6

moving from CaloSubdetectorGeometry to HcalGeometry. Code changes look OK, similar to the other modifications of this kind being made recently. The TauDiscriminationAgainstCaloMuon is currently not used in standard workflows
jenkins tests pass and comparisons show no differences

cmsbuild · 2017-09-18T17:34:03Z

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)

slava77 · 2017-09-25T13:49:05Z

@davidlange6
do you expect some changes for this PR
that prevent it from getting merged?
Note that the discussion in #20523 (comment) refers to a pattern of changes spread over many PRs, some already merged. So, perhaps an improvement to the geom interface can be done in a followup.

slava77 · 2017-09-28T13:58:05Z

@davidlange6
do you expect some changes for this PR
that prevent it from getting merged?

bsunanda · 2017-10-03T18:39:16Z

@slava77 @davidlange6 Please approve this - I do not foresee any improvement in near future

davidlange6 · 2017-10-13T16:17:47Z

+1

Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon

c16db58

cmsbuild added this to the CMSSW_9_4_X milestone Sep 14, 2017

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 14, 2017

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 14, 2017

Take care of code checker

077cc51

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 15, 2017

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 15, 2017

Take care of code checker

eaa68e6

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 15, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 15, 2017

cmsbuild added tests-started and removed tests-pending labels Sep 15, 2017

cmsbuild added tests-approved and removed tests-started labels Sep 15, 2017

cmsbuild added comparison-available and removed comparison-pending labels Sep 15, 2017

slava77 reviewed Sep 18, 2017

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Sep 18, 2017

davidlange6 mentioned this pull request Oct 2, 2017

Run2-hcx154 Make correct usage of HCAL geometry #20443

Closed

cmsbuild added orp-approved and removed orp-pending labels Oct 13, 2017

cmsbuild merged commit 42f293a into cms-sw:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run2-hcx155 Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon #20523

Run2-hcx155 Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon #20523

bsunanda commented Sep 14, 2017

cmsbuild commented Sep 14, 2017

cmsbuild commented Sep 14, 2017

davidlange6 commented Sep 14, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

perrotta commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

bsunanda commented Sep 15, 2017

cmsbuild commented Sep 15, 2017 •

edited

Loading

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

slava77 Sep 18, 2017

davidlange6 Sep 19, 2017

bsunanda Sep 19, 2017

davidlange6 Sep 20, 2017

bsunanda Oct 3, 2017

slava77 commented Sep 18, 2017

cmsbuild commented Sep 18, 2017

slava77 commented Sep 25, 2017

slava77 commented Sep 28, 2017

bsunanda commented Oct 3, 2017

davidlange6 commented Oct 13, 2017

Run2-hcx155 Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon #20523

Run2-hcx155 Correct access to HcalGeometry from TauDiscriminationAgainstCaloMuon #20523

Conversation

bsunanda commented Sep 14, 2017

cmsbuild commented Sep 14, 2017

cmsbuild commented Sep 14, 2017

davidlange6 commented Sep 14, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

davidlange6 commented Sep 15, 2017

perrotta commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

bsunanda commented Sep 15, 2017

cmsbuild commented Sep 15, 2017 • edited Loading

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

cmsbuild commented Sep 15, 2017

slava77 Sep 18, 2017

Choose a reason for hiding this comment

davidlange6 Sep 19, 2017

Choose a reason for hiding this comment

bsunanda Sep 19, 2017

Choose a reason for hiding this comment

davidlange6 Sep 20, 2017

Choose a reason for hiding this comment

bsunanda Oct 3, 2017

Choose a reason for hiding this comment

slava77 commented Sep 18, 2017

cmsbuild commented Sep 18, 2017

slava77 commented Sep 25, 2017

slava77 commented Sep 28, 2017

bsunanda commented Oct 3, 2017

davidlange6 commented Oct 13, 2017

cmsbuild commented Sep 15, 2017 •

edited

Loading