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

nbits_ was not set in TrackletLUT.cc for all tables. This is fixed now. #211

Closed aryd closed 1 year ago

aryd commented 1 year ago

PR description:

In TrackletLUT.cc the variable nbits was not set for all different LUTs and this caused a failure when trying to write out the LUTs to files. nbits is now set and code has been added to catch if nbits_ is not set and generate a meaning full error.

PR validation:

This code has been tested to verify that the writing of the LUTs work OK.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

aryd commented 1 year ago

OK - I ran “scram b -j code-format” and pushed the changes. (Can we add this to the wiki page - I try to follow the instructions there, but keep forgetting this…)

-Anders

Anders Ryd @.**@.>

On Mar 6, 2023, at 5:08 AM, Ian Tomalin @.**@.>> wrote:

@tomalin commented on this pull request.


In L1Trigger/TrackFindingTracklet/src/TrackletLUT.cchttps://github.com/cms-L1TK/cmssw/pull/211#discussion_r1126191020:

@@ -235,33 +235,43 @@ void TrackletLUT::initmatchcut(unsigned int layerdisk, MatchType type, unsigned name = settings.combined() ? "MP" : "MC";

if (type == barrelphi) {

This PR failed git CI, because "scram b -j code-format" was not run before making it.

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/211#discussion_r1126191020, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTJHUEU74TOSF3JJMMTW2WZQLANCNFSM6AAAAAAVQQV6ZI. You are receiving this because you authored the thread.Message ID: @.***>

aryd commented 1 year ago

Ian,

The twiki does not document the LUT formats. But I think it should I would like to add this. But I would like to do this only for the CombinedModules where we have fewer LUTs. In fact, all the enties that I updated were for the “standard” chain and I would like to remove all this code as soon as possible. The number of bits are defined by the code here - but we should have this documented. I will make this a reasonably high priority.

-Anders

Anders Ryd @.**@.>

On Mar 6, 2023, at 5:04 AM, Ian Tomalin @.**@.>> wrote:

@tomalin commented on this pull request.


In L1Trigger/TrackFindingTracklet/src/TrackletLUT.cchttps://github.com/cms-L1TK/cmssw/pull/211#discussion_r1126186959:

@@ -235,33 +235,43 @@ void TrackletLUT::initmatchcut(unsigned int layerdisk, MatchType type, unsigned name = settings.combined() ? "MP" : "MC";

if (type == barrelphi) {

Where do the numerical values, such as "10" that nbits_ is assigned to come from? Can they be deduced from https://twiki.cern.ch/twiki/bin/view/CMS/HybridDataFormat ? Are these numbers of bits not already stored in the Settings.h file, so we can could take them from there?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/211#pullrequestreview-1325876832, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTNL4T7PN3DG4UTQAGTW2WZCRANCNFSM6AAAAAAVQQV6ZI. You are receiving this because you authored the thread.Message ID: @.***>

tomalin commented 1 year ago

OK - I ran “scram b -j code-format” and pushed the changes. (Can we add this to the wiki page - I try to follow the instructions there, but keep forgetting this…) -Anders Anders Ryd @.**@.> On Mar 6, 2023, at 5:08 AM, Ian Tomalin @.**@.>> wrote: @tomalin commented on this pull request. ____ In L1Trigger/TrackFindingTracklet/src/TrackletLUT.cc<#211 (comment)>: @@ -235,33 +235,43 @@ void TrackletLUT::initmatchcut(unsigned int layerdisk, MatchType type, unsigned name = settings.combined() ? "MP" : "MC"; if (type == barrelphi) { + nbits_ = 10; This PR failed git CI, because "scram b -j code-format" was not run before making it. — Reply to this email directly, view it on GitHub<#211 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTJHUEU74TOSF3JJMMTW2WZQLANCNFSM6AAAAAAVQQV6ZI. You are receiving this because you authored the thread.Message ID: @.***>

Thanks. It's already documented on the twiki https://twiki.cern.ch/twiki/bin/view/CMS/L1TrackCodeRules .

tomalin commented 1 year ago

@aehart can't merge, as after last commit, it now fails CI ...

aehart commented 1 year ago

@aehart can't merge, as after last commit, it now fails CI ...

@aryd @tomalin The problem was that there is no case to handle type == VMRTableType::innerthird here, where nbits_ is set in initVMRTable: https://github.com/cms-L1TK/cmssw/blob/80b772e92b0743b41d4f32f10ec31a065fbe0749/L1Trigger/TrackFindingTracklet/src/TrackletLUT.cc#L1117-L1178

But this is also where name_ is set, and writeTable immediately returns if name_ is empty, so I've just moved the check of name_ to before the check of nbits_: https://github.com/cms-L1TK/cmssw/blob/c8cef028963e81a07a3af9d6ddb105322a72cfa3/L1Trigger/TrackFindingTracklet/src/TrackletLUT.cc#L1398-L1411