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.

Sign up for GitHub

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

Jump to bottom

Reco updates -- Backport btagging changes to 62X. #963

Merged
merged 8 commits into from Oct 2, 2013
Merged

Reco updates -- Backport btagging changes to 62X. #963

merged 8 commits into from Oct 2, 2013

Conversation

ktf
Copy link
Contributor

@ktf ktf commented Oct 1, 2013

Fix #892 conflict when merging.

@ktf ktf mentioned this pull request Oct 1, 2013
Closed
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2013

A new Pull Request was created by @ktf (Giulio Eulisse) for CMSSW_6_2_X.

Reco updates -- Backport btagging changes to 62X.

It involves the following packages:

RecoJets/Configuration
RecoJets/JetAssociationProducers
RecoBTag/ImpactParameter
.gitignore
FastSimulation/Configuration
RecoBTag/SoftLepton

@nclopezo, @smuzaffar, @thspeer, @giamman, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @TaiSakuma 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.
@davidlt you are the release manager for this.

@vadler
Copy link

vadler commented Oct 1, 2013

-1
even, if not requested from AT.
Reason: s. #892 (comment)
Solution: s, #892 (comment)

@ferencek
Copy link
Contributor

ferencek commented Oct 1, 2013

+1
as far as b tagging outside PAT is concerned.

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

ops... forgot your commit... Updating it now.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 2, 2013

Pull request #963 was updated. @smuzaffar, @nclopezo, @thspeer, @giamman, @slava77, @vadler can you please check and sign again.

@vadler
Copy link

vadler commented Oct 2, 2013

@ktf you did not rebase. That commit was originally for 70X.

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

? I cherry picked it. You said the #892 (comment) that 510ddb4 should be fine in 62X as well. What do you mean exactly?

@vadler
Copy link

vadler commented Oct 2, 2013

Sorry for being unprecise. I better replace my (possibly too quick) conclusion by the symptoms: Using CMSSW_7_0_X_2013-10-02-0200, I got plenty of merge conflicts now, like e.g.

Auto-merging CalibCalorimetry/EcalCorrectionModules/src/EcalGlobalShowerContainmentCorrectionsVsEtaESProducer.cc
CONFLICT (content): Merge conflict in CalibCalorimetry/EcalCorrectionModules/src/EcalGlobalShowerContainmentCorrectionsVsEtaESProducer.cc
Auto-merging CalibCalorimetry/CastorCalib/src/CastorDbXml.cc
CONFLICT (content): Merge conflict in CalibCalorimetry/CastorCalib/src/CastorDbXml.cc
Auto-merging CalibCalorimetry/CastorCalib/src/CastorDbHardcode.cc
CONFLICT (content): Merge conflict in CalibCalorimetry/CastorCalib/src/CastorDbHardcode.cc

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

I'm not sure I understand, we need to do this for 62X, right? How is this pull request related to 70x? Yes, it will probably have to be rebased there, something like:

git rebase --onto CMSSW_7_0_X ceb069c cce2136

but this is unrelated, no?

@vadler
Copy link

vadler commented Oct 2, 2013

510ddb4 was the commit to solve the problem in 70X. The same problem appears here, the same code change fixes it, that's all.
You put that commit in, and now I see a loooong list of merge conflicts, which makes me suspect that somehow a merge attempt of 70X into 62X happens in the background, but I don't know for sure.
You said, you cherry-picked that commit, and the resulting cce2136 looks indeed fine, so the problem is possibly elsewhere.

@vadler
Copy link

vadler commented Oct 2, 2013

Aahhhhh, it's in fact on my side. I should test the thing in 62X, not 70X (blush). Sorry for the noise!

@vadler
Copy link

vadler commented Oct 2, 2013

+1

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

That's what I was not understanding...:) If everyone else can check this out I can merge it and we proceed with the build.

@vadler
Copy link

vadler commented Oct 2, 2013

You suspected some subtile technical special? But it was simply about the confusion I ended up in, when testing five topics in three release cycles in parallel ;-)

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

I'm all for dropping 2 out of the 3 release cycles... ;)

ktf added a commit that referenced this pull request Oct 2, 2013
@ktf
Merge pull request #963 from ktf/fix-892-conflict
ebf1e61 
Reco updates -- Backport btagging changes to 62X.
@ktf ktf merged commit ebf1e61 into cms-sw:CMSSW_6_2_X Oct 2, 2013
@slava77
Copy link
Contributor

slava77 commented Oct 2, 2013

@thspeer is actually testing
(@slava77 testing, just in case)

@thspeer
Copy link
Contributor

thspeer commented Oct 2, 2013

+1
Tested ebf1e61 in CMSSW_6_2_X_2013-10-02-0200-963
Differences seen as expected in the b-tag results.
No other change

@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2013

Merging this @giamman, I'm in any case waiting for your approval.

@ktf ktf deleted the fix-892-conflict branch October 2, 2013 15:20
@giamman
Copy link
Contributor

giamman commented Oct 3, 2013

+1
but beware, I didn't test it (I am on vacation, checking my mails through an internet point, no possibility to open a shell), I just inspected the changes to the FamosSequences by eye.

3 similar comments
@giamman
Copy link
Contributor

giamman commented Oct 3, 2013

+1
but beware, I didn't test it (I am on vacation, checking my mails through an internet point, no possibility to open a shell), I just inspected the changes to the FamosSequences by eye.

@giamman
Copy link
Contributor

giamman commented Oct 3, 2013

+1
but beware, I didn't test it (I am on vacation, checking my mails through an internet point, no possibility to open a shell), I just inspected the changes to the FamosSequences by eye.

@giamman
Copy link
Contributor

giamman commented Oct 3, 2013

+1
but beware, I didn't test it (I am on vacation, checking my mails through an internet point, no possibility to open a shell), I just inspected the changes to the FamosSequences by eye.

@giamman
Copy link
Contributor

giamman commented Oct 3, 2013

+1
but beware, I didn't test it (I am on vacation, checking my mails through an internet point, no possibility to open a shell), I just inspected the changes to the FamosSequences by eye.

Andrea Giammanco
Phone (CERN): +41 22 76 71567
Mobiles: +41 76 2323672 (CH), +32 497 135121 (BE), +39 349 5552471 (IT)

From: Giulio Eulisse [notifications@github.com]
Sent: 02 October 2013 17:20
To: cms-sw/cmssw
Cc: Andrea Giammanco
Subject: Re: [cmssw] Reco updates -- Backport btagging changes to 62X. (#963)

Merging this @giammanhttps://github.com/giamman, I'm in any case waiting for your approval.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/963#issuecomment-25547632.

@slava77
Copy link
Contributor

slava77 commented Oct 4, 2013

@ktf
Giulio,
Since we all gave +1. Shouldn't the bot update the status of signatures? Currently this pull request has reco and fastsim as pending

@fabiocos
Copy link
Contributor

fabiocos commented Oct 4, 2013

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
analysis-approved fastsim-pending orp-pending pending-signatures reconstruction-pending tests-pending
Projects
None yet
Milestone
CMSSW_6_2_2
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@ktf @cmsbuild @vadler @ferencek @slava77 @thspeer @giamman @fabiocos @rappoccio