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

Seed Extension #12517

Merged
merged 4 commits into from Nov 25, 2015
Merged

Seed Extension #12517

merged 4 commits into from Nov 25, 2015

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Nov 20, 2015

Implement a Seed Extension as a TrajectoryFilter that requires no missing (or even no Invalid) hits as long as the track have less then a given number of hits beyond the seed.
The main purpose is (yet another way) to implement "Quadruplet seeding" for PhaseI.
It can also be used to reduce fakes (and speedup processing) in many other situations,
notably HLT.
Its indiscriminate use at RunII (i.e. changing the default) speeds-up tracking of almost a factor 2 at high pileup with minor loss in efficiency.

@VinInn
Copy link
Contributor Author

VinInn commented Nov 20, 2015

@cmsbuild, please test

@cmsbuild cmsbuild added this to the Next CMSSW_8_0_X milestone Nov 20, 2015
@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9881/console

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_8_0_X.

It involves the following packages:

RecoEgamma/EgammaPhotonProducers
TrackingTools/GsfTracking
TrackingTools/TrajectoryFiltering

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @istaslis, @gpetruc, @cerati, @lgray, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@VinInn I see quite a few changes in jenkins comparisons.
Please clarify more specifically the kind of changes expected from this PR default behavior
and please provide some standard tracking pre-validation plots.
Thanks

@VinInn
Copy link
Contributor Author

VinInn commented Nov 24, 2015

Sorry, @slava77 , could you point me to the most relevant differences?
I see none or few bins here and there compatible with usual float-math...

in any case, expected changes: NONE

@slava77
Copy link
Contributor

slava77 commented Nov 24, 2015

It's clearly not "NONE", not literally.
Some differences in 25202 triggered my question, e.g.
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-11-19-2300+12517/11467/validateJR/all_OldVSNew_TTbarPUwf25202p0/all_OldVSNew_TTbarPUwf25202p0c_log10recoGsfTracks_electronGsfTracks__RECO_obj_pt.png
I did not inspect this for significance, just assumed if no change is expected, then any change is significant.

@VinInn
Copy link
Contributor Author

VinInn commented Nov 24, 2015

Ok, I understand the change and I should correct the NONE.
conversion and gsf now clone the default configuration of the filter, while in the previous one they where defining their own PSET using therefore the defaults in the code which differs from those in the config.

Trk-POG considers this new behavior correct. I agree that my previous statement was wrong.
Up to you to confirm and consider this change in electron and photon an improvement or ask a separate PR.

@slava77
Copy link
Contributor

slava77 commented Nov 24, 2015

There is a one-track difference in generalTracks as well
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-11-19-2300+12517/11467/validateJR/all_OldVSNew_TTbarPUwf25202p0/all_OldVSNew_TTbarPUwf25202p0c_log10recoTracks_generalTracks__RECO_obj_pt.png
It's a tiny diff, but still: which iteration to blame here?

Please get the regular tracking (pre)validation plots.

@VinInn
Copy link
Contributor Author

VinInn commented Nov 24, 2015

here is mtv for ttbar 15PU 25ns 1000 events...
http://innocent.home.cern.ch/innocent/RelVal/pu15_seedex/
I observe one two tracks difference in muonseeds. Note that the reference dates Nov.7

@slava77
Copy link
Contributor

slava77 commented Nov 24, 2015

@cmsbuild please test
[even though the last update is trivial]

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9935/console

explicit SeedExtensionTrajectoryFilter() {}

explicit SeedExtensionTrajectoryFilter(edm::ParameterSet const & pset, edm::ConsumesCollector&) :
theStrict(pset.existsAs<bool>("strictSeedExtension") ? pset.getParameter<bool>("strictSeedExtension"):false),
Copy link
Contributor

Choose a reason for hiding this comment

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

what prevents making fillDescriptions for these?
Use of pset.existsAs in new code is an exception that needs to be justified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a PSet...
@makortel looked into this kind of issue last year, he may comment further (there is also an issue with PSet_Ref)
In any case with @mtosi we will review the use in HLT and eventually remove all those "exitsAs" that I dislike as well.
At the moment we need them if we with to progress fast (as HLT asked)

Copy link
Contributor

Choose a reason for hiding this comment

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

So let me repeat the story (again, and maybe some day we get there). The inherent "problem" is that the tracking code is organized as EDProducers that make extensive use of helper plugins (that use plugins that use plugins; by very quick look I can indeed find three layers of these...), and I'm not aware of any "easy" solution for fillDescription for plugins. E.g. the TrajectoryFilters here are such plugins.

One option suggested by @Dr15Jones
https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3446/1.html
would be to create a second class structures only for the fillDescriptions (my impression of the thread above is that the validation system does not currently understand refToPSet_). Back when we decided to replace the ESProducers with helper plugins and refToPSet_ we had many other alternatives on the table
https://indico.cern.ch/event/308199/contribution/8/2/attachments/588340/809738/slides.pdf
https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3244/4/1.html
I'm repeating these because one of the options back then was to store the helper algorithms to event (EDProducers could naturally use fillDescriptions), but downsides being either

  • no finer-grained caching than event (e.g. seed), or
  • rewriting algorithms to separate the transient state and pass that around
  • (or we could try to see if all fine-grained caching is still needed, I don't anymore remember which pieces of code do it)

Clearly whatever we will for the fillDescriptions in tracking will require significant development investment (possibly having impact on HLT configuration), and then somebody need(s) to decide what to do and when to do it.

As for HLT and existsAs, why not employing customizeHLTforCMSSW.py?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 25, 2015

+1

for #12517 d1c1731

  • changes in the code are in line with the PR description. A very small fraction of tracks is expected to change.
  • jenkins tests pass and in several workflows there are small differences at a single-track level
  • tested locally with higher statistics to confirm that the effect on the current tracking is small: looks like in well under 0.1% (per number of tracks) there are some insignificant differences.
    • e.g. in 1313 (3TeV dijet) workflow there are 0.01% more tracks and about the same fraction has different track parameters (a different trajectory is picked up?)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @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

5 participants