PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… #26130

hatakeyamak · 2019-03-09T02:32:10Z

…defining it in particleFlowSimParticle

PR description:

Fixing RecoParticleFlow/PFProducer/python/particleFlowSimParticle_cfi.py
It now reads ParticleFilter setting from
FastSimulation.Event.ParticleFilter_cfi
rather than re-defining it manually.

I expect no change in the reconstruction output. It just helps the PF hadron calibration workflow .

PR validation:

Made sure that now PF hadron calibration workflow works without over-writing settings for particleFlowSimParticle.

if this PR is a backport please specify the original PR:

This is not a backport.

…defining it in particleFlowSimParticle

cmsbuild · 2019-03-09T02:32:42Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-09T02:39:19Z

The code-checks are being triggered in jenkins.

slava77 · 2019-03-09T02:41:57Z

RecoParticleFlow/PFProducer/python/particleFlowSimParticle_cfi.py

@@ -35,3 +29,8 @@
    #retrieving fastSim SimHits                                     
    fastSimProducer = cms.untracked.InputTag('fastSimProducer','EcalHitsEB')
 )
+
+from FastSimulation.Event.ParticleFilter_cfi import ParticleFilterBlock


this conflicts with #26016

I see. I will just wait until 26016 converges then? This one is not urgent.

OK.
@Dr15Jones
did you have a timeline in mind for resolving #26016
?

#26061 is awaiting reconstruction review. Once that pull request is merged, I was going to move FastSimulation/BaseParticlePropagator to CommonTools/BaseParticlePropagator. That solves almost all the dependencies.

It does NOT break the dependency of FastSimulation on PFSimParticleProducer since that is using more of the 'heart' of fast simulation (it is also a 'Sim' related module).

@Dr15Jones
Did I read your last message correctly that a new dependence in this PR of RecoParticleFlow/PFProducer/python/particleFlowSimParticle_cfi on FastSimulation/Event is OK and will not actually conflict with activities related/expected from the resolution of #26016 "Break Reconstructions dependency on FastSimulation"?

I was only looking at build time dependencies, not python ones. Therefore this should have no effect on that restructuring. Whether or not adding such a dependency here is a good idea is another question.

Sounds good. We can then proceed with the review of this PR.
@hatakeyamak are you done with your edits in this PR?

cmsbuild · 2019-03-09T02:48:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26130/8695

This PR adds an extra 12KB to repository

cmsbuild · 2019-03-09T02:49:12Z

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

RecoParticleFlow/PFProducer

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray, @seemasharmafnal, @bachtis, @cbernet 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

cmsbuild · 2019-03-09T03:08:23Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-09T03:19:22Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26130/8696

This PR adds an extra 12KB to repository

cmsbuild · 2019-03-09T03:19:53Z

Pull request #26130 was updated. @cmsbuild, @perrotta, @slava77 can you please check and sign again.

hatakeyamak · 2019-03-09T14:52:35Z

@seemasharmafnal @spandeyehep

slava77 · 2019-03-12T16:21:05Z

RecoParticleFlow/PFProducer/python/particleFlowSimParticle_cfi.py

+
+# Custom setting
+particleFlowSimParticle.ParticleFilter.chargedPtMin = cms.double(0.0)
+particleFlowSimParticle.ParticleFilter.EMin = cms.double(0.0)


please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.

slava77 · 2019-03-12T16:24:12Z

RecoParticleFlow/PFProducer/python/particleFlowSimParticle_cfi.py

-        EMin = cms.double(0.0)
-    ),
+    #
+    ParticleFilter = ParticleFilterBlock.ParticleFilter,


I'm not sure that this leads to a full copy of FastSimulation.Event.ParticleFilter_cfi.ParticleFilterBlock
Please use a copy or make sure that the edits applied below are not modifying the values at the source.

perhaps

ParticleFilter = ParticleFilterBlock.ParticleFilter.clone(chargedPtMin = 0, EMin = 0)

is a simpler alternative (I didn't check that this syntax works)

cmsbuild · 2019-03-12T17:34:55Z

The code-checks are being triggered in jenkins.

hatakeyamak · 2019-03-12T17:36:01Z

Thanks @slava77 . I made the suggested change.

slava77 · 2019-03-12T17:59:23Z

@cmsbuild please test

cmsbuild · 2019-03-12T18:08:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26130/8750

This PR adds an extra 16KB to repository

cmsbuild · 2019-03-12T18:08:27Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33551/console Started: 2019/03/12 19:09

cmsbuild · 2019-03-12T18:08:29Z

Pull request #26130 was updated. @cmsbuild, @perrotta, @slava77 can you please check and sign again.

cmsbuild · 2019-03-12T20:05:18Z

+1
Tested at: f670b8a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26130/33551/summary.html

cmsbuild · 2019-03-12T20:05:26Z

Comparison job queued.

cmsbuild · 2019-03-12T21:23:13Z

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

Comparison Summary:

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

slava77 · 2019-03-12T22:38:58Z

+1

for #26130 f670b8a

code changes are in line with the PR description and the follow up review.
- The original configuration in particleFlowSimParticle_cfi.py was simply broken: it looks like it was originally broken in April of 2016 in 81X with FastSim: clean up and simplify generator particle filtering #14266 after a change in the configuration of the KineParticleFilter and then diverged even more in September of 2018 with FastSim: make two parameters of the KineParticleFilter configurable #24473
- particleFlowSimParticle_cfi.py is not used in the production setup, only in a few /test cfgs, which are not running regularly.
jenkins tests pass and comparisons with the baseline show no differences

cmsbuild · 2019-03-12T22:39:27Z

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

fabiocos · 2019-03-14T09:33:42Z

+1

import ParticleFilter setting from pre-defined config rather than re-…

51823f5

…defining it in particleFlowSimParticle

cmsbuild added this to the CMSSW_10_6_X milestone Mar 9, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 9, 2019

hatakeyamak changed the title ~~import ParticleFilter setting from pre-defined config rather than re-…~~ PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… Mar 9, 2019

...

c0596de

slava77 reviewed Mar 9, 2019

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 9, 2019

Cleanup

d250103

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 9, 2019

fix

92d7e35

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 9, 2019

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 9, 2019

slava77 reviewed Mar 12, 2019

View reviewed changes

improve syntax

f670b8a

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 12, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 12, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 12, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 12, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Mar 12, 2019

cmsbuild added orp-approved and removed orp-pending labels Mar 14, 2019

cmsbuild merged commit 1d376f5 into cms-sw:master Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… #26130

PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… #26130

hatakeyamak commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

slava77 Mar 9, 2019

hatakeyamak Mar 9, 2019 •

edited

slava77 Mar 9, 2019

Dr15Jones Mar 9, 2019

slava77 Mar 9, 2019

Dr15Jones Mar 9, 2019

slava77 Mar 9, 2019

hatakeyamak Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

hatakeyamak commented Mar 9, 2019

slava77 Mar 12, 2019

slava77 Mar 12, 2019

cmsbuild commented Mar 12, 2019

hatakeyamak commented Mar 12, 2019

slava77 commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019 •

edited

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

slava77 commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

fabiocos commented Mar 14, 2019

PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… #26130

PFSimParticle: import ParticleFilter setting from pre-defined config rather than re-… #26130

Conversation

hatakeyamak commented Mar 9, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

Choose a reason for hiding this comment

hatakeyamak Mar 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

cmsbuild commented Mar 9, 2019

hatakeyamak commented Mar 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Mar 12, 2019

hatakeyamak commented Mar 12, 2019

slava77 commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019 • edited

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

slava77 commented Mar 12, 2019

cmsbuild commented Mar 12, 2019

fabiocos commented Mar 14, 2019

hatakeyamak Mar 9, 2019 •

edited

cmsbuild commented Mar 12, 2019 •

edited