search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

MP HLS agreement fixes #126

Closed aryd closed 2 years ago

aryd commented 2 years ago

PR description:

This PR includes some code changes to the MatchProcessor that was developed in Dec. 2021 to get full agreement between the HLS and emulations for the MatchProcessor

PR validation:

Has verified that this agrees with the HLS code.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

tschuh commented 2 years ago

I am confused to see code changes in this PR which got already merged. Could you please try to rebase onto L1TK-dev-12_0_0_pre4? Or tell me which commit are actually new?

aryd commented 2 years ago

I tried following the instructions on the twiki page:

_(b) If you have created your own branch (as you intend making a PR), then in new CMSSW project area, (this is checked by git expert):

git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4 git cms-rebase-topic -u cms-L1TK:myBranch

When I apply this I get:

@.*** TrackFindingTracklet]$ git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4 From .

Which looks OK, but then:

@.*** TrackFindingTracklet]$ git cms-rebase-topic -u cms-L1TK:MP_HLS_Agreement_Fixes_220131 From .

Was this not the right instructions to follow to rebaseline?

-Anders

Anders Ryd @.**@.>

On Feb 1, 2022, at 12:42 PM, tschuh @.**@.>> wrote:

I am confused to see code changes in this PR which got already merged. Could you please try to rebase onto L1TK-dev-12_0_0_pre4? Or tell me which commit are actually new?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/126#issuecomment-1027114116, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPX5FV3Z24RSEXPA3TUZALOTANCNFSM5NJZVP3A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.***>

tomalin commented 2 years ago

@aryd Your MP_HLS_Agreement_Fixes branch contains, as Thomas points out, the changes to FPGAWord.cc that were in his merged PR https://github.com/cms-L1TK/cmssw/pull/125 . How did they get into your branch? Did you create your branch after Thomas's PR was merged? Or did you copy his changes into your branch by hand? Or did you create your branch before Thomas's PR was merged, but then rebase and push the rebased code to your branch?

Also when you attempted a rebase above, did you remember to execute "git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4" in an empty CMSSW src/ directory? I just tried this, and I do not get printout like "M L1Trigger/TrackFindingTracklet/interface/Settings.h", which to me suggest that you already had code present, which was modified by the checkout.

tschuh commented 2 years ago

When I follow the instructions with a fresh checkout git asks me to resolve merge conflicts after git cms-rebase-topic -u cms-L1TK:MP_HLS_Agreement_Fixes_220131. I suggest you try it again from a fresh enviroment. Alternatively you can try this git reset --hard cms-L1TK/L1TK-dev-12_0_0_pre4 after git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4 assuming you have added the remote beforehand with git remote add cms-L1TK git@github.com:cms-L1TK/cmssw.git This should ensure that your local L1TK-dev-12_0_0_pre4 branch corresponds exactly to the remote version.

aryd commented 2 years ago

This was done in the same working area as where I fixed the various < to <= etc for Thomas FPGAWord.cchttp://FPGAWord.cc changes. In this area I pushed my changes to Thomas' branch for the FPGAWord fix. I then created a new branch, MP_HLS_Agreeement_Fixes_220131, for the fixes to the MatchProcessor. Thinking that git should support multiple branches without checking out all code from scratch…

Indeed, I did miss the " in new CMSSW project area”… (Is this really needed?)

Anyhow, I went through the steps of creating a new area. After fixing various conflicts in the rebase I think I manage to push the changes to the MP_HLS_Agreement_Fixes_220131. (But now I should still test this and I will need to remake the other changes that I did not want to commit that I had in the old area…)

-Anders

Anders Ryd @.**@.>

On Feb 1, 2022, at 2:14 PM, Ian Tomalin @.**@.>> wrote:

@arydhttps://github.com/aryd Your MP_HLS_Agreement_Fixes branch contains, as Thomas points out, the changes to FPGAWord.cchttp://FPGAWord.cc that were in his merged PR #125https://github.com/cms-L1TK/cmssw/pull/125 . How did they get into your branch? Did you create your branch after Thomas's PR was merged? Or did you copy his changes into your branch by hand? Or did you create your branch before Thomas's PR was merged, but then rebase and push the rebased code to your branch?

Also when you attempted a rebase above, did you remember to execute "git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4" in an empty CMSSW src/ directory? I just tried this, and I do not get printout like "M L1Trigger/TrackFindingTracklet/interface/Settings.h", which to me suggest that you already had code present, which was modified by the checkout.

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/126#issuecomment-1027195932, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTLMBLWXTIOQ3FVVMITUZAWJXANCNFSM5NJZVP3A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.Message ID: @.***>

bryates commented 2 years ago

This was done in the same working area as where I fixed the various < to <= etc for Thomas FPGAWord.cchttp://FPGAWord.cc changes. In this area I pushed my changes to Thomas' branch for the FPGAWord fix. I then created a new branch, MP_HLS_Agreeement_Fixes_220131, for the fixes to the MatchProcessor. Thinking that git should support multiple branches without checking out all code from scratch… Indeed, I did miss the " in new CMSSW project area”… (Is this really needed?) Anyhow, I went through the steps of creating a new area. After fixing various conflicts in the rebase I think I manage to push the changes to the MP_HLS_Agreement_Fixes_220131. (But now I should still test this and I will need to remake the other changes that I did not want to commit that I had in the old area…) -Anders Anders Ryd @.**@.> On Feb 1, 2022, at 2:14 PM, Ian Tomalin @.**@.>> wrote: @arydhttps://github.com/aryd Your MP_HLS_Agreement_Fixes branch contains, as Thomas points out, the changes to FPGAWord.cchttp://FPGAWord.cc that were in his merged PR #125<#125> . How did they get into your branch? Did you create your branch after Thomas's PR was merged? Or did you copy his changes into your branch by hand? Or did you create your branch before Thomas's PR was merged, but then rebase and push the rebased code to your branch? Also when you attempted a rebase above, did you remember to execute "git cms-checkout-topic -u cms-L1TK:L1TK-dev-12_0_0_pre4" in an empty CMSSW src/ directory? I just tried this, and I do not get printout like "M L1Trigger/TrackFindingTracklet/interface/Settings.h", which to me suggest that you already had code present, which was modified by the checkout. — Reply to this email directly, view it on GitHub<#126 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTLMBLWXTIOQ3FVVMITUZAWJXANCNFSM5NJZVP3A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.Message ID: @.***>

If you want to clean up the history, it might be better to start with a new branch off of L1TK-dev-12_0_0_pre4, and use git cherry-pick SHA to grab the specific new commits from this PR.

bryates commented 2 years ago

I just did a rebase with L1TK-dev-12_0_0_pre4 and force pushed. Hopefully this fixes the rebase issue that was causing the CI to fail.