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

fix propagator in SeedFromConsecutiveHitsCreator config file #8464

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Mar 23, 2015

RecoTracker/TkSeedGenerator/python/SeedFromConsecutiveHitsCreator_cfi.py
had a not coherent default setting (propagator <----> SimpleMagneticField)

@VinInn @rovere : why RecoTracker/TkSeedGenerator/plugins/SeedFromConsecutiveHitsCreator.h/cc in 75x is not in synch w/ 74x ?

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mtosi (mia tosi) for CMSSW_7_5_X.

fix propagator in SeedFromConsecutiveHitsCreator config file

It involves the following packages:

RecoTracker/TkSeedGenerator

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @mschrode, @gpetruc, @cerati, @dgulhan, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@@ -1,9 +1,9 @@
import FWCore.ParameterSet.Config as cms

Bimport FWCore.ParameterSet.Config as cms
Copy link
Contributor

Choose a reason for hiding this comment

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

spurious character?
also next line????

@cmsbuild
Copy link
Contributor

Pull request #8464 was updated. @cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please check and sign again.

@makortel
Copy link
Contributor

@mtosi These files were modified in #8390 (74X)/#8393 (75X) and the 74X was merged first in CMSSW_7_4_X_2015-03-20-2300, while the 75X in CMSSW_7_5_X_2015-03-22-2300. Maybe you chose IBs within this period? In the HEADs of both 74X and 75X all the three .cc, .h, and _cfi,py files are equivalent.

@slava77
Copy link
Contributor

slava77 commented Mar 23, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@makortel
Copy link
Contributor

@boudoul Bringing this also to your attention.

Following the same logic, we should undo this in the Phase1 configurations (in order to achieve compatibility with SLHC), i.e.

<module>.SeedCreatorPSet.propagator = "PropagatorWithMaterial"

in RecoTracker/IterativeTracking/python/Phase1*.py. Since your planned PR (fixing the SimpleMagneticField -> magneticField in Phase1) will touch the same locations in the same files, I see three possible ways out

  1. Mia (or e.g. me) does the changes here (may result in merge conflicts with your PR depending on what gets merged and when)
  2. You do the changes in your PR
  3. Somebody (me?) does the changes after both PRs have been merged

Any preferences? (I'm currently leaning towards 2)

@mtosi
Copy link
Contributor Author

mtosi commented Mar 23, 2015

the code in this IB [CMSSW_7_5_X_2015-03-18-0200]
is somehow old !
look for instance at the following file
https://github.com/mtosi/cmssw/blob/from-CMSSW_7_5_X_2015-03-18-0200_fixSeedFromConsecutiveHitsCreator/RecoTracker/TkSeedGenerator/plugins/SeedFromConsecutiveHitsCreator.h

@boudoul
Copy link
Contributor

boudoul commented Mar 23, 2015

Hi @markotel, I just discovered this exchange, among the three possible ways out you were listing :

  1. Mia (or e.g. me) does the changes here (may result in merge conflicts with your PR depending on what gets merged and when)
  2. You do the changes in your PR
  3. Somebody (me?) does the changes after both PRs have been merged

I will choose 2)
Thanks for bringing my attention to this

@mtosi
Copy link
Contributor Author

mtosi commented Mar 23, 2015

ciao

looking @ PR #8393
I see that the file RecoTracker/TkSeedGenerator/plugins/SeedFromConsecutiveHitsCreator.cc is coherent w/ the current version in 74x,
while in this PR it is somehow obsolete

=> I would close this PR w/o merging it,
while asking to @boudoul to update the config file (thanks much)

@makortel @VinInn @rovere

@makortel
Copy link
Contributor

@mtosi It shouldn't matter if the SeedFromConsecutiveHitsCreator.cc in the "base IB of this PR" is "out of date". The change in configuration is needed nevertheless, right? (or do we rely on the automatic forward port of #8465?)

In the above (#8464 (comment)) I was only discussing on how to react on the Phase1 side to this change.

@mtosi
Copy link
Contributor Author

mtosi commented Mar 23, 2015

@makortel
this change is needed, right
because I'm always confused on the automatic forward port of the code,
I preferred to make explicitly this PR

what still is not clear to me is how the c++ will be ported in 75x, though

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2015

+1

for #8464 e9b346d
tested locally in CMSSW_7_4_0_pre9 /test area sign533/
changes overall are less significant than what we had in the #2291

Most distributions change without any particular patter (generalTracks are affected less, conversions and gsf tracks are changing more).
Plots with the most visible changes are for the initial and lowPtTriplet step seeding
(the following plots are for 202.0 Run1 ttbar with PU; but the same behavior is observed in Run2 setup [25202.0] as well as in Run1 data processing [4.53])
wf202_initialstepseeds_seed_eta
wf202_lowpttriplet_seed_eta

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

@davidlange6
Copy link
Contributor

+1

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

7 participants