Fixes to produce the combined module MemPrint and LUT files for a red… #182
Conversation
* Add disk cut tables * Removed debug lines * code-format * Fixed LUTs * Disk LUTs for MP * Initialize LUTs * Added comments to TrackletLUT.h * code-format
* Trying best cut values * Added header, clang-format
|
@aryd this fails CI as you didn't run "scram b -j8 code-format" (and check by eye that the format changes look sensible). |
|
Done. I have pushed the updated code now.
-Anders
Anders Ryd
***@***.******@***.***>
On Aug 15, 2022, at 5:11 PM, Ian Tomalin ***@***.******@***.***>> wrote:
@aryd<https://github.com/aryd> this fails CI as you didn't run "scram b -j8 code-format" (and check by eye that the format changes look sensible).
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTLNZY5H42UNJBI7JFTVZJM3HANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Please add comment to top of TrackletLUT.h explaining what this class is for. |
|
Please add a comment describing any function in TrackletLUT.h, whose purpose is not obvious from its name. |
* fix for stubs with identical position but different bends. * Update L1FPGATrackProducer.cc
* Move module loop to setup * Import functions from trackerTFP * Ian's 2nd comment * Code-format Co-authored-by: Jack Li <[email protected]>
|
@aryd this PR seems to have stalled? I'm waiting for my comments to be addressed before approving it. |
Fixed comment in code
|
Note from L1TrkAlgo meeting. Anders believes many of the constants were in the code before this PR. He will fix them, but not his top priority, so may take a while. The PR cant be merged until then. |
* CalcBendCuts - Uses bend encoding to decode bend * Modified CalcBendCuts to be off by default, changed nzbinsPhiCorr to 1 by default * Added changed in TP LUT and set default nzbinsPhiCorr to 1 * Turned off CalcBendCuts and revised comments * PR Cleanup * More PR cleanup * Address PR comments * code-format
Done |
Done |
Done |
Have been addressed |
* kf bug fixes, f/w sync. * updated reduced dat files and channel assignment. * correct channel assignment. * reduced dat files deleted.
|
@aryd this needs rebasing to resolve the git conflicts mentioned above. |
* These changes implements a LUT as function of the raidal projection in the disks to determine which r bins needs to be searched - and the finer position within the r bin * Added extra pipeline stage to match HLS * Ran code-format * Remove some hard-coded numbers * Fixed parentheses * code-format * Minor fixes to remove some hardcoded numbers * Run code-format Co-authored-by: Anders <[email protected]>
|
Ian,
I think I have completed the merge. The code is running and the results seems OK. Let me know if you see any issues.
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2022, at 5:47 AM, Ian Tomalin ***@***.******@***.***>> wrote:
@aryd<https://github.com/aryd> The conflicts with TrackletLUT.cc<http://TrackletLUT.cc> are not trivial. They arise as your PR #182<#182> and @bryates<https://github.com/bryates> PR (now merged) #185<#185> both made significant changes to this code.
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTJQOS22KW4LQDVK6OLWHDM3PANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@bryates Please take a look through class TrackletLUT in Ander's branch of PR 182 https://github.com/cms-L1TK/cmssw/tree/ReducedCombinedModuleFixes , and check that he hasn't accidentally deleted any changes that you made, which remain important. |
|
@aryd This git PR is still complaining "This branch has conflicts that must be resolved". In addition https://github.com/cms-L1TK/cmssw/tree/ReducedCombinedModuleFixes/L1Trigger/TrackFindingTracklet/src says that TrackletLUT.cc in your branch has not been changed for 12 days. Therefore, if you attempted to fix it 1 hour ago, I think your changed was not pushed. |
Looks like all the changes are just comments, and some variables to replace constants. I'm happy with these changes. |
Hi @bryates Sorry, I think you must wait until Anders has eliminated the git conflict reported in this PR before checking what he's done to your code ... |
|
I think I have resolved the git conflicts? No?
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2022, at 11:29 AM, Ian Tomalin ***@***.******@***.***>> wrote:
@bryates<https://github.com/bryates> Please take a look through class TrackletLUT in Ander's branch of PR 182 https://github.com/cms-L1TK/cmssw/tree/ReducedCombinedModuleFixes , and check that he hasn't accidentally deleted any changes that you made, which remain important.
Looks like all the changes are just comments, and some variables to replace constants. I'm happy with these changes.
Hi @bryates<https://github.com/bryates> Sorry, I think you must wait until Anders has eliminated the git conflict reported in this PR before checking what he's done to your code ...
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTLUB4PSIS57DIIYYD3WHEU5HANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hi Anders, I'm afraid the git conflicts are still present. Search for the sentence "This branch has conflicts that must be resolved" in #182 . |
|
Ian, I followed the instructions on the wiki page to resolve the conflicts. I think this went fine. However, I get this message when I try to push the changes:
***@***.*** test]$ git push cms-L1TK ReducedCombinedModuleFixes
Enter passphrase for key '/afs/cern.ch/user/a/aryd/.ssh/id_rsa':<http://cern.ch/user/a/aryd/.ssh/id_rsa':>
To ***@***.******@***.***>:cms-L1TK/cmssw.git
! [rejected] ReducedCombinedModuleFixes -> ReducedCombinedModuleFixes (non-fast-forward)
error: failed to push some refs to ***@***.******@***.***>:cms-L1TK/cmssw.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
I can make a pull - but I really don’t understand why this would be needed. There has not been any changes in the remote for 13 days and I did the merge yesterday. Is this some side effect of doing the conflict resolving?
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2022, at 10:52 AM, Ian Tomalin ***@***.******@***.***>> wrote:
This branch has conflicts that must be resolved
Hi Anders, I'm afraid the git conflicts are still present. Search for the sentence "This branch has conflicts that must be resolved" in #182<#182> .
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTPU423T7MZ4VAUDRDDWHEXUFANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hi @aryd , after doing a rebase, you must use "git push -f cms-L1TK yourBranchName". This is documented in https://twiki.cern.ch/twiki/bin/viewauth/CMS/L1TrackSoftware , if you search for section "https://twiki.cern.ch/twiki/bin/viewauth/CMS/L1TrackSoftware" and then for text "You need to do "git push -f ..." when done. |
|
OK I made a backup and went through the git pull etc. Now I think this is pushed. (But I find the way git does this super confusing as I when I did the git pull it again had to do a merge of the changes in TrackletLUT.{cc,h} this time it did it automatically.)
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 8, 2022, at 6:42 AM, Anders Ryd ***@***.******@***.***>> wrote:
Ian, I followed the instructions on the wiki page to resolve the conflicts. I think this went fine. However, I get this message when I try to push the changes:
***@***.*** test]$ git push cms-L1TK ReducedCombinedModuleFixes
Enter passphrase for key '/afs/cern.ch/user/a/aryd/.ssh/id_rsa':<http://cern.ch/user/a/aryd/.ssh/id_rsa':>
To ***@***.******@***.***>:cms-L1TK/cmssw.git
! [rejected] ReducedCombinedModuleFixes -> ReducedCombinedModuleFixes (non-fast-forward)
error: failed to push some refs to ***@***.******@***.***>:cms-L1TK/cmssw.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
I can make a pull - but I really don’t understand why this would be needed. There has not been any changes in the remote for 13 days and I did the merge yesterday. Is this some side effect of doing the conflict resolving?
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2022, at 10:52 AM, Ian Tomalin ***@***.******@***.***>> wrote:
This branch has conflicts that must be resolved
Hi Anders, I'm afraid the git conflicts are still present. Search for the sentence "This branch has conflicts that must be resolved" in #182<#182> .
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTPU423T7MZ4VAUDRDDWHEXUFANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hi @aryd, I fear your rebase went wrong. #182 says you have changed a total of 46 files, whereas only TrackletLUT was in conflict, and you had only changed a few files before the rebase. I suggest you use my recipe below, to remove your bad rebase, starting in an empty CMSSW area: #If you made a mistake and added a bad commit the end of your branch and push it to it, then:
|
|
I think the rebase went fine. I resolved the issued in TrackletLUT.{cc,h}. The code built and ran fine. The results looked as expected.
I think that were things must have gone wrong is in the ‘git pull’ step.
After merging and checking that the code worked I tried to push, but was told I needed to do ‘git pull’ I tried this, but got an error that no branch was specified so I tried
git pull ReducedCombinedModuleFixes
Then it complains that I did not have a remote so I tried:
git pull cms-L1TK ReducedCombinedModuleFixes
This merged the code, but the automatic merging seemed to have worked.
Then I did , git add, git commit, git push
But I now see that this did indeed create merge conflicts. I resolved these - this involved making again the same set of edits as I did 2 or 3 times during the rebaselining. There must be something wrong with the process here…
I tried to compile and run the code and it seems fine.
I then did the usual git add, git commit, and git push
I think this should now be OK.
I also noticed that during the git commit I got a new message:
***@***.*** TrackFindingTracklet]$ git commit -m "Fix the same merge conflicts yet another time"
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
But I presume this is just informational.
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 8, 2022, at 12:11 PM, Ian Tomalin ***@***.******@***.***>> wrote:
Hi @aryd<https://github.com/aryd>, I fear your rebase went wrong. #182<#182> says you have changed a total of 46 files, whereas only TrackletLUT was in conflict, and you had only changed a few files before the rebase.
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTJX3ODLSGRWMSQCYOTWHKJT3ANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hi @aryd , I suspect that you were doing fine until you reached the step "After merging and checking that the code worked I tried to push, but was told I needed to do ‘git pull’". This git message is very misleading (as git messages often are). The true reason your git push failed is that you needed to do "git push -f" (always required after rebase). I suspect that the "git pull cms-L1TK ReducedCombinedModuleFixes" messed things up, and explains why this PR now has so many changed files. The git CI fails complaining that TrackletLUT is still in conflict https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/4734209 . An illustration of the issue is that looking at the "Files changed" for this PR, one sees that it adds "namespace tt {class Setup}" to line 17 of Settings.h https://github.com/cms-L1TK/cmssw/pull/182/files#diff-a60861ca738c293df9cfb82e85a7dec56e53e508645e3b3a361be1f9dc21c041 , despite the fact that these lines were already present in our default development branch https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_5_0_pre2/L1Trigger/TrackFindingTracklet/interface/Settings.h#L17 . |
|
I have a tar file after the rebase, but before I did the ‘git pull’. Do you think I can just do a “git push -f” from what I saved?
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 8, 2022, at 2:20 PM, Ian Tomalin ***@***.******@***.***>> wrote:
Hi @aryd<https://github.com/aryd> ,
I suspect that you were doing fine until you reached the step "After merging and checking that the code worked I tried to push, but was told I needed to do ‘git pull’".
This git message is very misleading (as git messages often are). The true reason your git push failed is that you needed to do "git push -f" (always required after rebase). I suspect that the "git pull cms-L1TK ReducedCombinedModuleFixes" messed things up, and explains why this PR now has so many changed files.
The git CI fails complaining that TrackletLUT is still in conflict https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/4734209 .
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTKAEOQFKV2VXY2J5GDWHKYYNANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hi @aryd, that might work. It's worth trying. If it fails, then if you can give me the versions of TrackletLUT.cc and .h that have all conflicts resolved, I can try making a PR with these myself (if you like). |
|
Ian,
I do see that the “git pull -f” was indeed in the instructions. But I followed the advice from git…
My tar file was made only for the TrackFindingTracklet directly and not the ‘root’ of the git area so without doing something more complicated I can not just push from this area.
If you can take the files from here:
~aryd/public/TrackletLUT.*
And make a PR it would be great.
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 8, 2022, at 2:36 PM, Ian Tomalin ***@***.******@***.***>> wrote:
I have a tar file after the rebase, but before I did the ‘git pull’. Do you think I can just do a “git push -f” from what I saved? -Anders Anders Ryd @.@.> On Nov 8, 2022, at 2:20 PM, Ian Tomalin @.@.>> wrote: Hi @aryd<https://github.com/aryd>https://github.com/aryd , I suspect that you were doing fine until you reached the step "After merging and checking that the code worked I tried to push, but was told I needed to do ‘git pull’". This git message is very misleading (as git messages often are). The true reason your git push failed is that you needed to do "git push -f" (always required after rebase). I suspect that the "git pull cms-L1TK ReducedCombinedModuleFixes" messed things up, and explains why this PR now has so many changed files. The git CI fails complaining that TrackletLUT is still in conflict https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/4734209 . — Reply to this email directly, view it on GitHub<#182 (comment)<#182 (comment)>>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTKAEOQFKV2VXY2J5GDWHKYYNANCNFSM552PNG7A. You are receiving this because you were mentioned.Message ID: @.***>
Hi @aryd<https://github.com/aryd>, that might work. It's worth trying. If it fails, then if you can give me the versions of TrackletLUT.cc<http://TrackletLUT.cc> and .h that have all conflicts resolved, I can try making a PR with these myself (if you like).
—
Reply to this email directly, view it on GitHub<#182 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTNWN6T7SRRBZQIGGODWHK2VHANCNFSM552PNG7A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Conclusion: as we're in a git mess here, I've created a fresh PR with Anders's versions of TrackletLUT.cc and .h at #189 . |
…uced project
PR description:
This PR has two fixes to produced the LUTs and MemPrint tar files for the reduced combined module configuration. There is a minor fix in VMRouterCM to protect agains not writing the AllStubs memory. (In the reduced configuration this memory is not written for L1 and L2 where the seeing is done.) This also write out all LUT files in the .dat format, i.e. HEX numbers. This simplifies the reading of the files in hdl.
PR validation:
Files were generated and tested by running the reduced combined module chain in HLS.
if this PR is a backport please specify the original PR and why you need to backport that PR:
N/A
Before submitting your pull requests, make sure you followed this checklist: