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

Me0 muons in 76 x commitv2 #11398

Merged
merged 46 commits into from Oct 7, 2015

Conversation

dnash86
Copy link
Contributor

@dnash86 dnash86 commented Sep 21, 2015

This is the PR for dedicated ME0Muons for 76X. A previous PR was closed for us to test things before reopening, this is the same MEMuon functionality, now tested.

Most comments from the previous PR have been incorporated/addressed, moving the location of a few modules and some other changes as well. One comment regarding a dummy plane hasn't been implemented, we plan to send a PR for ME0 chambers in full reco soon as well. Naturally there might be further comments.

@dnash86
Copy link
Contributor Author

dnash86 commented Sep 21, 2015

Github tells me there are conflicts that must be merged. I've tried to fix it, but when I try to cms-merge-topic the branch it doesn't complain of any merge conflicts, and just gives me one warning:
"error: addinfo_cache failed for path 'HLTriggerOffline/Exotica/python/analyses/hltExoticaPFHT_cff.py'"

I'm not sure how to solve this, so I sent the PR. In the past when there is a conflict, trying merge-topic showed warnings for them, but I'm not sure where the conflict is now, any advice?

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dnash86 for CMSSW_7_6_X.

Me0 muons in 76 x commitv2

It involves the following packages:

CommonTools/RecoAlgos
DataFormats/MuonReco
RecoLocalMuon/GEMRecHit
RecoMuon/MuonIdentification
SLHCUpgradeSimulations/Configuration
SimMuon/GEMDigitizer

@cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @battibass, @makortel, @abbiendi, @jhgoh, @Martin-Grunewald, @ahinzmann, @bellan, @trocino, @amagitte, @bachtis, @rociovilar 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.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Sep 22, 2015

@dnash86
which IB or pre release did you use to make this PR?

@dnash
Copy link

dnash commented Sep 22, 2015

@slava77 I did it in CMSSW_7_6_0_pre4.

I tried to check for conflicts by doing cms-merge-topic in a recent IB, I believe it was CMSSW_7_6_X_2015-09-21-1100, and found this error but wasn't sure what to do:

"error: addinfo_cache failed for path 'HLTriggerOffline/Exotica/python/analyses/hltExoticaPFHT_cff.py'"

@slava77
Copy link
Contributor

slava77 commented Sep 22, 2015

On 9/22/15 9:23 AM, dnash wrote:

@slava77 https://github.com/slava77 I did it in CMSSW_7_6_0_pre4.

I tried to check for conflicts by doing cms-merge-topic in a recent IB,
I believe it was CMSSW_7_6_X_2015-09-21-1100, and found this error but
wasn't sure what to do:

"error: addinfo_cache failed for path
'HLTriggerOffline/Exotica/python/analyses/hltExoticaPFHT_cff.py'"

You can ignore this error.

I think your conflicts are with more recent changes (most likely 10353
or similar)

you can wait until 09-22-2300 IB shows up or update to it now by a rebase
in your current area
(typing offhand, somewhat sure it will work)

git fetch official-cmssw CMSSW_7_6_X:CMSSW_7_6_X
git checkout ME0Muons-In-76X_commitv2
git checkout -b ME0Muons-In-76X_commitv2-rebX
git rebase --onto=CMSSW_7_6_X CMSSW_7_6_0_pre4
git push my-cmssw -f ME0Muons-In-76X_commitv2-rebX:ME0Muons-In-76X_commitv2

this should do the work and push the rebase into the same PR and update it


Reply to this email directly or view it on GitHub
#11398 (comment).

@dnash
Copy link

dnash commented Sep 22, 2015

@slava77 Thanks a lot for the help with that, I made a mistake executing your instructions the first time, but it seems to be okay now (possible to be merged).

@cmsbuild
Copy link
Contributor

Pull request #11398 was updated. @cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please check and sign again.

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@slava77
Copy link
Contributor

slava77 commented Oct 6, 2015

this unit test is failing in the IB as well
https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc493/CMSSW_7_6_X_2015-10-05-2300/unitTestLogs/RecoMET/METProducers
It can be ignored for this PR

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2015

@slava77
Copy link
Contributor

slava77 commented Oct 6, 2015

David,
I simplified your test config, which was too surgical and hard to maintain:
you can cherry-pick 3d07963 on top of your PR
you can fetch the commit from slava77:CMSSW_7_6_0_pre6/sign604/pull11398-fixRunTest

The version that I have committed runs on the output produced from

  • RecoLocalMuon/GEMRecHit/test/SingleMuPt100_cfi_GEM-SIM_Extended2015MuonGEMDev_RPCGEMME0Customs_cfg.py
  • RecoLocalMuon/GEMRecHit/test/SingleMuPt100_cfi_DIGI_Extended2015MuonGEMDev_RPCGEMME0Customs_cfg.py

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2015

+1

for #11398 1c3e42f

  • code changes look ok: ME0 detector local (and partly global) reconstruction are ported to 76X; a somewhat working config file was provided and can be run to test the new modules (timing on single gun events is small, as expected for no-PU situation)
  • jenkins tests pass and comparisons with baseline as expected
    • the failing unit test is also failing in the IBs, unrelated to this PR
  • updated config file can come in a separate PR

@dnash86
Copy link
Contributor Author

dnash86 commented Oct 7, 2015

@slava77 Thanks for the "+1", I take it you're requesting that I send a PR for the changes to the config that you made separately (by itself in a PR)?

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2015

On 10/6/15 8:07 PM, dnash86 wrote:

@slava77 https://github.com/slava77 Thanks for the "+1", I take it
you're requesting that I send a PR for the changes to the config that
you made separately (by itself in a PR)?

You can update this PR as well.
But a separate PR may make it easier for a much larger code base in this
PR to make it in pre7 on time
(today)


Reply to this email directly or view it on GitHub
#11398 (comment).

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2015

@civanch please check this PR
your signature is needed.
Thank you.

@civanch
Copy link
Contributor

civanch commented Oct 7, 2015

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2015

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (but tests are reportedly failing). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

davidlange6 added a commit that referenced this pull request Oct 7, 2015
@davidlange6 davidlange6 merged commit 0a86c68 into cms-sw:CMSSW_7_6_X Oct 7, 2015
@slava77
Copy link
Contributor

slava77 commented Oct 7, 2015

@dnash86
about my suggested modifications: you can avoid having to apply 4 lines with "useLumiInfoRunHeader" if you add the pileup info product in the digi step (load SimGeneral/PileupInformation/python/AddPileupSummary_cfi.py and add addPileupInfo in the digi sequence).

Then the reco part becomes almost trivial: you just use the official reco and add the me0 modules.

@dnash86
Copy link
Contributor Author

dnash86 commented Oct 7, 2015

@slava77 Okay, thanks, I will look at your suggestions and try implementing them. The reason for the complex test file was ECAL related errors due to the piecemeal geometry.

@dnash86
Copy link
Contributor Author

dnash86 commented Oct 7, 2015

Did you mean to edit the DIGI file like so:

process.load ('SimGeneral.PileupInformation.AddPileupSummary_cfi')
process.digitisation_step = cms.Path(process.pdigi*process.addPileupInfo)

For me if I just do that, it crashes on the digi step with:
Looking for type: PileupVertexContent

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2015

On 10/7/15 2:53 PM, dnash86 wrote:

Did you mean to edit the DIGI file like so:

process.load ('SimGeneral.PileupInformation.AddPileupSummary_cfi')
process.digitisation_step = cms.Path(process.pdigi*process.addPileupInfo)

Yes, I was hoping it was just that.

For me if I just do that, it crashes on the digi step with:
Looking for type: PileupVertexContent


Reply to this email directly or view it on GitHub
#11398 (comment).

@slava77
Copy link
Contributor

slava77 commented Oct 8, 2015

... about the addPileupInfo in the digi step: it looks like it's killed by the gem customs both in the sequence and in the event content.
So, recovery of both is needed.

I have updated my branch (and dropped the "Timing" printouts to make it more quiet)
slava77/cmssw@0bfa10d...slava77:CMSSW_7_6_0_pre6/sign604/pull11398-fixRunTest

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

8 participants