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

backport HLT developments from 9.2.x #20235

Closed

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Aug 22, 2017

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_9_0_X.

It involves the following packages:

DataFormats/EgammaReco
HLTriggerOffline/JetMET
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaHLTProducers
RecoMuon/GlobalTrackingTools
RecoMuon/L3TrackFinder
RecoMuon/TrackerSeedGenerator
RecoTracker/TkDetLayers
SimMuon/MCTruth
SimTracker/TrackerHitAssociation
Validation/RecoMuon

@perrotta, @cmsbuild, @civanch, @vazzolini, @kmaeshima, @fwyzard, @mdhildreth, @dmitrijus, @Martin-Grunewald, @silviodonato, @slava77, @vanbesien can you please review it and eventually sign? Thanks.
@ghellwig, @felicepantaleo, @jainshilpi, @abbiendi, @echapon, @lgray, @threus, @battibass, @makortel, @sdevissc, @jhgoh, @dgulhan, @HuguesBrun, @trocino, @rociovilar, @Sam-Harper, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @wmtford, @mschrode, @ebrondol, @mtosi, @amagitte, @calderona, @gpetruc, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

* \author D.Chamont, U.Berthon, C.Charlot, LLR Palaiseau
*
************************************************************/
#ifndef DataFormats_EgammaReco_ElectronSeed_h
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is probably the only change that triggers long recompilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it is.

Unrelated to this there is a lot of other phaseI stuff needed to be compiled (its not just this E/gamma change which creates the situation David was complaining about).

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 22, 2017 via email

@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 23, 2017

If I understood correctly, this is the full request - it just is not only about E/Gamma.

I can change the subject if you prefer, of course.

@Sam-Harper
Copy link
Contributor

Ah yes, there is more than just e/gamma here (I got confused from the title). I dont know what else is needed so wont comment on that. This has the e/gamma changes

@slava77
Copy link
Contributor

slava77 commented Aug 23, 2017

@fwyzard
a more complete title would be better so that it covers changes in this PR, at least with a broad brush

@fwyzard fwyzard changed the title backport E/Gamma developments from 9.2.x backport HLT developments from 9.2.x Aug 23, 2017
@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-20235/22572/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 106 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1738728
  • DQMHistoTests: Total failures: 6075
  • DQMHistoTests: Total nulls: 2360
  • DQMHistoTests: Total successes: 1730130
  • DQMHistoTests: Total skipped: 163
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 2, 2017

What was the baseline in 92X for the backport and was it actually a backport (as parts of the diffs look like new developments)?

I notice that the following do not exist in 92X or master and appear to be new developments that should be in master first

  • SimMuon/MCTruth/plugins/HLTFilterToTrackProducer in entirety
  • additions in Validation/RecoMuon/plugins/MuonTrackValidator
  • change in Validation/RecoMuon/python/MuonTrackValidator_cfi.py in maxPt
  • changes in RecoMuon/TrackerSeedGenerator/plugins/TSGForOI.cc

Then more:

@slava77
Copy link
Contributor

slava77 commented Sep 7, 2017

@fwyzard @Sam-Harper
please clarify on the origin of the additional changes as mentioned in #20235 (comment)
Thank you.

@Sam-Harper
Copy link
Contributor

@slava77 , sorry, no idea. I took the latest 92X /93X for the electron stuff but thats not what you're asking out.

@slava77
Copy link
Contributor

slava77 commented Sep 7, 2017

Looking more carefully, it looks like the author of other commits is Carlo.
@battibass
please comment on the origin of the commits for this backport,
more specifically related to #20235 (comment).
The changes are now proposed as a "backport from 92X", but some include new developments not yet in any release.

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2017

@fwyzard
please clarify the status of this PR.

@fwyzard
Copy link
Contributor Author

fwyzard commented Sep 11, 2017

This is what was requested to further the HLT studies for Phase 2, based on the reports from @Sam-Harper @dsperka @battibass .
Personally, I cannot add anything nor comment their request.

If necessary, @gennai and marco.pieri@cern.ch can follow this up and comment further.

@gennai
Copy link
Contributor

gennai commented Sep 11, 2017

Apart from repeating what has already written by Andrea (i.e. these are backporting needed for upgrade studies) I cannot say much more.
About the technical comment on the missing code in the master,
I have pinged on skype @battibass who promised to answer asap.

1 similar comment
@gennai
Copy link
Contributor

gennai commented Sep 11, 2017

Apart from repeating what has already written by Andrea (i.e. these are backporting needed for upgrade studies) I cannot say much more.
About the technical comment on the missing code in the master,
I have pinged on skype @battibass who promised to answer asap.

@battibass
Copy link

Hi all,
firstly apologies for the extremely belated reply.
Getting to the comments:

  1. TSGForOI and MuonTrackingRegionBuilder: the new Muon L3 code was developed quite actively after I forked to make it run in 2023, it can/should be rebased (my changes are minor).
  2. needed Validation changes: what is necessary is in SimTracker/TrackerHitAssociation/ (this has a significant amount of dependencies, hence it is worth a backport) plus the relevant cfi change to make the muon associator switch to phase2 gemetry in SimMuon/MCTruth/python/MuonAssociatorByHits_cfi.py.
  3. not needed Validation changes: HLTFilterToTrackProducer is just an handy tool and the change in maxpT and the additional plots in MuonTrackValidator.cc are analysis level changes of the workflow that can be ignored.

Said so, @fwyzard, how would you suggest to proceed (e.g. do you want me to get your branch, test the changes and make a PR to your github that you can integrate here)?

@slava77
Copy link
Contributor

slava77 commented Sep 19, 2017

[This is a weekly check.]
@fwyzard @battibass @gennai
please clarify on the status of this PR
Based on the last messages here I expect

  • updates in this PR to rebase to a more recent 92X version of the muon code
  • PR(s) for new necessary code not yet in master

@fwyzard
Copy link
Contributor Author

fwyzard commented Sep 19, 2017

@battibass, I would suggest to

  • make a PR for any missing functionality for the master/9.4.x release, and make sure everything works there
  • make a PR to backport any missing code to 9.3.x
  • make a PR to backport the same changes to 9.0.x

For the last point, you can start from my branch, but if I remember correctly all the changes came form you in the first place ?

@gennai
Copy link
Contributor

gennai commented Sep 19, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Sep 25, 2017

ping, just as a weekly check

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2017

-1

to take this off the list of PRs that need imminent attention
updates are expected anyways to proceed

@Martin-Grunewald
Copy link
Contributor

-1
superseeded by #20725

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

9 participants