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

island photons for XeXe collision era #20929

Merged
merged 10 commits into from Oct 31, 2017

Conversation

ttrk
Copy link
Contributor

@ttrk ttrk commented Oct 16, 2017

fixes the pp_on_XeXe_2017 era to run island photons and add them into event content
original PR for pp_on_XeXe_2017 era : #20749

@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-20929/1473

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ttrk (Kaya Tatar) for master.

It involves the following packages:

RecoEcal/Configuration
RecoEgamma/Configuration
RecoEgamma/EgammaPhotonProducers
RecoHI/HiEgammaAlgos

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @varuns23, @rovere, @argiro, @yenjie, @jazzitup, @kurtejung, @lgray, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Oct 16, 2017

@cmsbuild please test workflow 148.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 16, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23790/console Started: 2017/10/16 17:57

@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-20929/23790/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: 2767139
  • DQMHistoTests: Total failures: 104
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2766864
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@@ -26,8 +26,14 @@
from Configuration.Eras.Modifier_pp_on_XeXe_2017_cff import pp_on_XeXe_2017

from RecoEcal.EgammaClusterProducers.islandBasicClusters_cfi import islandBasicClusters
from RecoHI.HiEgammaAlgos.HiIslandSuperClusters_cfi import islandSuperClusters
from RecoHI.HiEgammaAlgos.HiCorrectedIslandBarrelSuperClusters_cfi import correctedIslandBarrelSuperClusters
from RecoHI.HiEgammaAlgos.HiCorrectedIslandEndcapSuperClusters_cfi import correctedIslandEndcapSuperClusters
Copy link
Contributor

Choose a reason for hiding this comment

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

all of these 3 imports can collide with the same names possibly already imported.
This ambiguity should be removed.
More transparent way may be to apply era modifiers (.toReplaceWith) to the original definitions used in RecoEcal/EgammaClusterProducers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first option that comes to my mind is to do something like the following. Is that what you are asking for ?
from RecoEcal.EgammaClusterProducers.islandClusteringSequence_cff import islandClusteringSequence
_ecalClustersHI = ecalClusters.copy()
_ecalClustersHI += islandClusteringSequence

Copy link
Contributor

Choose a reason for hiding this comment

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

that will come as a consequence of the changes I proposed in the original definition location.

Another alternative is to change names of HI-specific islandSuperClusters and other modules to hiIslandSuperClusters and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an example for the proposed changes in the original definition location ? I want to make sure I understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ttrk : the point is that (for example) the module "islandSuperClusters" has the same name in both
​RecoEcal/​EgammaClusterProducers/​python/​islandSuperClusters_cfi.py
and
RecoHI/​HiEgammaAlgos/​python/​HiIslandSuperClusters_cfi.py
(and a different configuration).

Therefore, if you import one of them in this RecoEcal_cff.py you prevent importing the other one, or you end up with ambiguities in the configurations.

The solution could be to rename, e.g., hiIslandSuperClusters the module in HiIslandSuperClusters_cfi.py (and the same for the other modules), and then use the eras to add the correct modules either here (as you do now), or, probably better, inside the islandClusteringSequence, as you also suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@perrotta Thanks for the explanation. I understand the ambiguity resulting from that the modules in different files are given the same names.

However when @slava77 mentioned it as "Another alternative" (#20929 (comment)), it sounded to me as if renaming the HI-specific modules is an "alternative" way and one can solve the ambiguity without renaming modules.

Anyway I will rename the HI-specific modules.

Kaya Tatar added 2 commits October 25, 2017 02:25
…me" ambiguity with the modules under RecoEcal/EgammaClusterProducers/
@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-20929/24029/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: 2838442
  • DQMHistoTests: Total failures: 120
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2838151
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@perrotta
Copy link
Contributor

+1

  • It does the job of adding island photons to the reco output in XeXe era:
    0.0 -> 202.9 203 NEWO 0.01 recoPhotons_islandPhotons__RECO.
    0.0 -> 171.2 171 NEWO 0.01
    recoHIPhotonIsolationedmValueMap_photonIsolationHIProducerppIsland__RECO.
    (as well as to pA_2016 and peripheralPbPb eras)

@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)

@perrotta
Copy link
Contributor

@ttrk : you probably want to backport this PR to 94X, and 92X as well

Please prepare the backport PR(s) if so

ttrk pushed a commit to ttrk/cmssw that referenced this pull request Oct 30, 2017
apply the era modifiers to the original definitions in RecoEcal/EgammaClusterProducers
as suggested here cms-sw#20929 (comment)
and here cms-sw#20929 (comment)
@ttrk
Copy link
Contributor Author

ttrk commented Oct 30, 2017

made backport PRs
PR for 94X : #21069
PR for 92X : #21070

@ttrk
Copy link
Contributor Author

ttrk commented Oct 30, 2017

@perrotta : I need to make a new PR which adds an era for pp reference run. It will modify some of the files in this PR. Should I wait for this PR to be merged (to avoid conflicts) or do you recommend something else ?

@perrotta
Copy link
Contributor

Well, this relates to the question I made in the 92X backport of this PR....

If this PR, in its present form, need to enter quickly in 82X (or 94X) for the XeXe run reprocessing or whatever else reason, I'd suggest to have it merged (it is simple, and therefore it could be done in short, probably).

If this is just an intermediate step for the pp reference run reco, then you can even modify this PR itself, and then the review process will restart on it. In that case, I imagine that the 92X and 94X backport could be momentarily closed while this one converges.

So the question is the same: what are your plans for this PR?

@ttrk
Copy link
Contributor Author

ttrk commented Oct 30, 2017

@perrotta Can we merge this PR and the backports asap ? My branch for the new PR is more or less ready. If these are merged, then I can make the new PR with era for pp reference today/tomorrow.

@perrotta
Copy link
Contributor

Let check with @davidlange6 which are the plans for merging this PR and its backports (@ttrk please take however into account that backport merging normally requires at lest one IB cycle after the PR in the master is merged)


from RecoHI.HiEgammaAlgos.HiCorrectedIslandEndcapSuperClusters_cfi import correctedIslandEndcapSuperClusters as _hiCorrectedIslandEndcapSuperClusters

for e in [pA_2016, peripheralPbPb, pp_on_XeXe_2017]:
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there should be an easy way to avoid this ever lengthening set of modifiers and just have one.. I'll try to suggest something

Copy link
Contributor Author

@ttrk ttrk Oct 31, 2017

Choose a reason for hiding this comment

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

I will make a new PR which adds an era for 2017 pp ref run. I can implement your suggestion in the new PR.

@davidlange6
Copy link
Contributor

merge

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