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

Modernized DetIdAssociatorESProducer #27010

Merged
merged 2 commits into from Jun 8, 2019

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • removed framework constructs (e.g. ParameterSet and EventSetup records) from DetIdAssociators
  • created DetIdAssociatorMakers which get configurations from the ParameterSet, set what EventSetup data products will be consumed and then create the appropriate DetIdAssociator.

PR validation:

The code compiles.

This is intended to be a refactoring so should not change any results.

- removed framework constructs from DetIdAssociators
- created DetIdAssociatorMakers which get configurations from
  the ParameterSet, set what EventSetup data products will be
  consumed and then create the appropriate DetIdAssociator.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27010/10077

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

TrackingTools/TrackAssociator

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@battibass, @makortel, @felicepantaleo, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @calderona, @HuguesBrun, @ebrondol, @drkovalskyi, @gpetruc, @bellan, @rovere, @trocino, @dgulhan, @folguera 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

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 30, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/616/console Started: 2019/05/30 21:27

@cmsbuild
Copy link
Contributor

-1

Tested at: f95dfae

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cd0ae7/616/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found compilation error when building:

>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetSpecific.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetMatchingTools.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetIDHelper.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/CastorJetIDHelper.cc 
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetMuonHitsIDHelper.cc: In member function 'void reco::helper::JetMuonHitsIDHelper::calculate(const edm::Event&, const edm::EventSetup&, const reco::Jet&, int)':
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetMuonHitsIDHelper.cc:65:16: error: 'GlobalTrackingGeometryRecord' was not declared in this scope
     iSetup.get ().get(trackingGeometry);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-05-30-1100/src/RecoJets/JetProducers/src/JetMuonHitsIDHelper.cc:65:16: note: suggested alternative: 'GlobalTrackingGeometry'
     iSetup.get ().get(trackingGeometry);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3210678
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3210343
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@slava77
Copy link
Contributor

slava77 commented May 31, 2019

removed framework constructs (e.g. ParameterSet and EventSetup records) from DetIdAssociators

what was the problem with this?
Is there a rule somewhere that a tool delivered by an ES producer can not be constructed with a ParameterSet in the argument?

@Dr15Jones
Copy link
Contributor Author

what was the problem with this?
Is there a rule somewhere that a tool delivered by an ES producer can not be constructed with a ParameterSet in the argument?

From a maintenance standpoint, the less couplings the better. For example, we strongly discourage Event data products from knowing about the edm::Event or the parameter set system. I personally apply those same philosophies to data products in the EventSetup system as well.

@slava77
Copy link
Contributor

slava77 commented May 31, 2019

+1

for #27010 f8f515e

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences

@fabiocos
Copy link
Contributor

fabiocos commented Jun 3, 2019

@christopheralanwest @pohsun @santocch could you please check?

@pohsun
Copy link

pohsun commented Jun 7, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented Jun 8, 2019

this code it looks like will need clang-format cleaning

@Dr15Jones
Copy link
Contributor Author

@fabiocos can the formating be done in a separate pull request or does it have to be in this one?

@fabiocos
Copy link
Contributor

fabiocos commented Jun 8, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented Jun 8, 2019

merge

@Dr15Jones we will do it separately, I am just checking how much is left (or undone) in recent PRs

@cmsbuild cmsbuild merged commit 353069c into cms-sw:master Jun 8, 2019
@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

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 be automatically merged.

@Dr15Jones Dr15Jones deleted the consumesDetIdAssociatorESProducer branch June 14, 2019 18:05
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