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

Remove PixelTripletLowPtGenerator as obsolete #16886

Closed

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Dec 6, 2016

While migrating PixelTrackProducer to the new seeding framework (from #16635), I encountered AllPixelTracks_cfi configuration using PixelTripletLowPtGenerator. Neither of them is currently used by anything else, so this PR proposes to remove them as obsolete.

The removal of RecoPixelVertexing/PixelTrackFitting/test/test_cfg.py is not related to PixelTripletLowPtGenerator, but something additional I encountered that looked obsolete (and hence maybe not worth to be migrated). I included it here since it was also under RecoPixelVertexing.

Tested in CMSSW_9_0_X_2016-12-02-1100, no changes expected.

@rovere @VinInn

@makortel
Copy link
Contributor Author

makortel commented Dec 6, 2016

@cmsbuild, please test

@cmsbuild cmsbuild added this to the Next CMSSW_9_0_X milestone Dec 6, 2016
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16811/console Started: 2016/12/06 16:48

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_9_0_X.

It involves the following packages:

RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTrackFitting

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @dgulhan, @rovere, @VinInn this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@Martin-Grunewald
Copy link
Contributor

RecoPixelVertexing/PixelLowPtUtilities/python/AllPixelTracks_cfi.py
is used by ConfDB parsing.

@makortel
Copy link
Contributor Author

makortel commented Dec 6, 2016

@Martin-Grunewald Ok. Can you easily tell what information ConfDB parsing picks from the file that is not already elsewhere?

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Dec 6, 2016

ConfDB parsing is used to discover the parameters at top-level of the module class.
It is based on this cfi file for the PixelTrackProducer. The parameters within the
PSet parameters can be changed/added/subtracted, but the top-level parameters are fixed.

This results in:

process.hltPixelTracks = cms.EDProducer( "PixelTrackProducer",
    useFilterWithES = cms.bool( False ),
    FilterPSet = cms.PSet( 
      chi2 = cms.double( 1000.0 ),
      nSigmaTipMaxTolerance = cms.double( 0.0 ),
      ComponentName = cms.string( "PixelTrackFilterByKinematics" ),
      nSigmaInvPtTolerance = cms.double( 0.0 ),
      ptMin = cms.double( 0.1 ),
      tipMax = cms.double( 1.0 )
    ),
    passLabel = cms.string( "" ),
    FitterPSet = cms.PSet( 
      ComponentName = cms.string( "PixelFitterByHelixProjections" ),
      TTRHBuilder = cms.string( "hltESPTTRHBuilderPixelOnly" ),
      fixImpactParameter = cms.double( 0.0 )
    ),
    RegionFactoryPSet = cms.PSet( 
      ComponentName = cms.string( "GlobalRegionProducerFromBeamSpot" ),
      RegionPSet = cms.PSet( 
        precise = cms.bool( True ),
        originRadius = cms.double( 0.2 ),
        ptMin = cms.double( 0.9 ),
        originHalfLength = cms.double( 24.0 ),
        beamSpot = cms.InputTag( "hltOnlineBeamSpot" ),
        useMultipleScattering = cms.bool( False ),
        useFakeVertices = cms.bool( False )
      )
    ),
    CleanerPSet = cms.PSet(  ComponentName = cms.string( "PixelTrackCleanerBySharedHits" ) ),
    OrderedHitsFactoryPSet = cms.PSet( 
      ComponentName = cms.string( "StandardHitTripletGenerator" ),
      GeneratorPSet = cms.PSet( 
        useBending = cms.bool( True ),
        useFixedPreFiltering = cms.bool( False ),
        maxElement = cms.uint32( 100000 ),
        phiPreFiltering = cms.double( 0.3 ),
        extraHitRPhitolerance = cms.double( 0.06 ),
        useMultScattering = cms.bool( True ),
        SeedComparitorPSet = cms.PSet( 
          ComponentName = cms.string( "LowPtClusterShapeSeedComparitor" ),
          clusterShapeCacheSrc = cms.InputTag( "hltSiPixelClustersCache" )
        ),
        extraHitRZtolerance = cms.double( 0.06 ),
        ComponentName = cms.string( "PixelTripletHLTGenerator" )
      ),
      SeedingLayers = cms.InputTag( "hltPixelLayerTriplets" )
    )
)

If there is another cfi file with all top-level parameters as above, we could use that!

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

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

The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@makortel
Copy link
Contributor Author

makortel commented Dec 6, 2016

More "proper" cfi for PixelTrackProducer would be https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/RecoPixelVertexing/PixelTrackFitting/python/PixelTracks_cfi.py (it is used in workflows running pixel tracking offline). On the other hand, my #16792 and next-to-#16792 PRs will make PixelTrackProducer configuration look completely different (ok, #16792 actually migrates this file, but for its followup I decided to give up if possible), but you'll get cfi's for everything via fillDescriptions() (and complete "migration" in customizeHLTforCMSSW).

To be practical, since this PR is not urgently needed (it's mainly to answer beforehand possible questions on next-to-#16792 why I'm not migrating these pieces), it's probably easiest to wait until next-to-#16792 gets merged so that there'll be a fully consistent setup for the "new way" to be eventually parsed to ConfDB.

Could somebody "hold" this PR? (or is closing preferred?)

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 6, 2016

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2016

Pull request has been put on hold by @cvuosalo
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Dec 6, 2016
@slava77
Copy link
Contributor

slava77 commented Dec 16, 2016

@makortel #16792 is merged already.
Do we have a number for the post-#16792 PR mentioned in #16886 (comment) ?

@slava77
Copy link
Contributor

slava77 commented Dec 19, 2016

a bunch of merge conflicts has accumulated here already
Maybe it's time to close or at least rebase.

@makortel
Copy link
Contributor Author

@slava77

Do we have a number for the post-#16792 PR mentioned in #16886 (comment) ?

Not yet, because I'd prefer either #17041 or #17042 (HLT era support) to be merged first, because those would allow me easily to test the HLT-customize to avoid breaking it, and any development in the customize file (HLTrigger/Configuration/python/customizeHLTTrackingForPhaseI2017.py) would conflict with #17041/#17042.

a bunch of merge conflicts has accumulated here already
Maybe it's time to close or at least rebase.

I'll rebase and also "descope" (from deletion of PixelTripletLowPtGenerator to adding large "FIXME" comments on top of the files, because low-pT analyses may need it in the future, as I mentioned in the last reco meeting), probably tomorrow.

@slava77
Copy link
Contributor

slava77 commented Jan 10, 2017

@makortel
#17042 was merged two weeks ago (a couple of working days ago).
it may be time to update this to the desired (descoped) implementation.

@makortel
Copy link
Contributor Author

@slava77 I know, unfortunately this one has fallen a bit below of other (more useful) stuff and holidays. I'll try to update by the end of the week.

@davidlange6
Copy link
Contributor

hi @makortel - should we close this?

@makortel
Copy link
Contributor Author

Maybe indeed better close for now, as it continues to stay below other stuff (sorry for that). I'll make a new PR with updated content when I find a good time to do it.

@makortel makortel closed this Jan 27, 2017
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.

6 participants