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

Track Quality Emulation Update #201

Closed Chriisbrown closed 1 year ago

Chriisbrown commented 1 year ago

PR description:

This PR brings in a new ap_fixed version of the Track quality BDT which is also used in Firmware. It sees updates to the track quality in the TrackTrigger directory with a new runTQemulation class method that uses the ap_fixed datatypes and the new conifer.h header.

This PR also sees updates to the KFout with the track quality emulation included as well as updates to reflect previous updates to the track word class that has simplified the KFout. The distribution server for track routing has also been removed in place of a simpler routing logic.

To run a new BDT json file is needed located here: https://github.com/cms-L1TK/l1tk-for-emp/tree/kfout/tq/scripts and moved to L1Trigger/TrackTrigger/data

This version of the BDT is only an emulated version so accompanies the existing floating point BDT in the TrackQuality package. These previous versions should be retained to allow for studies with different BDTs. This emulated version in it's current form has only been tested and verified with the bit-accurate KF emulation but the runEmulatedTQ function could be inserted into other parts of the emulation if this is useful, but would require the inputs to the BDT to be converted to the ap_fixed data type.

PR validation:

This PR has been compared against Firmware in the kfout branch of the l1tk-for-emp repo in both modelsim simulation and physical hardware testing

tomalin commented 1 year ago

If L1Trigger/TrackTrigger/ now refers to ap_fixed, how come it's BuildFile.xml doesn't need the line

tomalin commented 1 year ago

Please clarify in the PR description, does this new code entirely replace on the old TrackQuality code (which has therefore been deleted)? And can the new TQ code be run either after the New KF or after the Old KF (which had non digital output)?

Chriisbrown commented 1 year ago

If L1Trigger/TrackTrigger/ now refers to ap_fixed, how come it's BuildFile.xml doesn't need the line

I am unsure why I am able to build and run the code without the ap_fixed in the BuildFile.xml, it might be being imported elsewhere, it could always be added to the BuildFile.xml to ensure there it always works

tomalin commented 1 year ago

This PR is failing CI checks. I notice that Thomas's PR which passes CI uses his branch https://github.com/cms-L1TK/cmssw/tree/tschuh_dr , whereas this PR which fails CI uses branch https://github.com/cms-L1TK/cmssw/tree/cms-L1TK/cbrown_tq_emu . I wonder if the fact that "cms-L1TK" appears twice in the branch name explains the problem?

Chriisbrown commented 1 year ago

This PR is failing CI checks. I notice that Thomas's PR which passes CI uses his branch https://github.com/cms-L1TK/cmssw/tree/tschuh_dr , whereas this PR which fails CI uses branch https://github.com/cms-L1TK/cmssw/tree/cms-L1TK/cbrown_tq_emu . I wonder if the fact that "cms-L1TK" appears twice in the branch name explains the problem?

image

Is there a nice way of changing the branch name without breaking this PR ?

tomalin commented 1 year ago

This PR is failing CI checks. I notice that Thomas's PR which passes CI uses his branch https://github.com/cms-L1TK/cmssw/tree/tschuh_dr , whereas this PR which fails CI uses branch https://github.com/cms-L1TK/cmssw/tree/cms-L1TK/cbrown_tq_emu . I wonder if the fact that "cms-L1TK" appears twice in the branch name explains the problem?

image

Is there a nice way of changing the branch name without breaking this PR ?

Not that I know. I suggest you answer all review comments above. And then create a new PR with new branch ...

Chriisbrown commented 1 year ago

If L1Trigger/TrackTrigger/ now refers to ap_fixed, how come it's BuildFile.xml doesn't need the line

I am unsure why I am able to build and run the code without the ap_fixed in the BuildFile.xml, it might be being imported elsewhere, it could always be added to the BuildFile.xml to ensure there it always works

I have looked further into this and in other places in CMSSW where the ap_fixed datatypes are used, namely the rest of the L1 trigger, and they also do not include the ap_fixed in their BuildFiles so I am unsure if it is needed, it would appear not

tomalin commented 1 year ago

If L1Trigger/TrackTrigger/ now refers to ap_fixed, how come it's BuildFile.xml doesn't need the line

I am unsure why I am able to build and run the code without the ap_fixed in the BuildFile.xml, it might be being imported elsewhere, it could always be added to the BuildFile.xml to ensure there it always works

I have looked further into this and in other places in CMSSW where the ap_fixed datatypes are used, namely the rest of the L1 trigger, and they also do not include the ap_fixed in their BuildFiles so I am unsure if it is needed, it would appear not

Hi, I'd suggest adding anyway. It doesn't do any harm, and makes people aware that this package is unusual in that it is using HLS types. And logically it is needed. If it doesn't fail without it, I guess it must be being included by one of the other packages you're including.

Chriisbrown commented 1 year ago

If there are no further comments, @tomalin, I will change the name of this branch and remake the PR?

tomalin commented 1 year ago

If there are no further comments, @tomalin, I will change the name of this branch and remake the PR?

Go ahead