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

Add track quality emulation to HYBRID_NEWKF #221

Closed tomalin closed 1 year ago

tomalin commented 1 year ago

PR description:

This is a copy of https://github.com/cms-L1TK/cmssw/pull/209 , rebased to the latest code from cms-L1TK:L1TK-dev-12_6_0_pre5 , created by Ian Tomalin to understand why CI is not running correctly on Chris's branch.

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

@Chriisbrown please make a PR to https://github.com/cms-data/L1Trigger-TrackTrigger to add there your file https://github.com/cms-L1TK/cmssw/blob/cbrown_tq_emu_rebased/L1Trigger/TrackTrigger/data/clf_GBDT_emulation_newKF_digitized.json . (I understand we can't yet remove the .onyx file from https://github.com/cms-data/L1Trigger-TrackTrigger , because it will be needed until the code in PR221 is merged into central CMSSW).

P.S. I never include the data/ directories in cms-L1TK in any PR we make to central CMSSW, since CMSSW doesn't include data/ directories. We only have them in cms-L1TK for convenience.

Chriisbrown commented 1 year ago

@Chriisbrown please make a PR to https://github.com/cms-data/L1Trigger-TrackTrigger to add there your file https://github.com/cms-L1TK/cmssw/blob/cbrown_tq_emu_rebased/L1Trigger/TrackTrigger/data/clf_GBDT_emulation_newKF_digitized.json . (I understand we can't yet remove the .onyx file from https://github.com/cms-data/L1Trigger-TrackTrigger , because it will be needed until the code in PR221 is merged into central CMSSW).

P.S. I never include the data/ directories in cms-L1TK in any PR we make to central CMSSW, since CMSSW doesn't include data/ directories. We only have them in cms-L1TK for convenience.

This PR is now here: https://github.com/cms-data/L1Trigger-TrackTrigger/pull/3

tomalin commented 1 year ago

@Chriisbrown I've added an "Issue" https://github.com/cms-L1TK/cmssw/issues/222 to remind us to remove the .onnx file from central CMSSW, at some point in the future, after the current cms-L1TK code is merged into it. Please check if it makes sense.

tomalin commented 1 year ago

@cgsavard as you reviewed the original copy of this PR https://github.com/cms-L1TK/cmssw/pull/201 , can you please confirm (e.g. by "approving" this one) that all your comments were implemented to your satisfaction?

Personally, I'm now happy with this PR.

cgsavard commented 1 year ago

@tomalin @Chriisbrown I am happy with all of the changes. The only thing I want to keep on people's radars is that the conifer.h file will eventually be somewhere more globally in CMSSW. Whenever this happens, we should remove our version and refer to that new version. Maybe we can also include this in an issue?

tomalin commented 1 year ago

222

Hi @cgsavard can you please add the conifer.h issue to https://github.com/cms-L1TK/cmssw/issues/222 and change its title accordingly? Who will tell us when conifer.h has been moved to a global location? Who's in charge of doing it?

cgsavard commented 1 year ago

Upon further investigation, I actually see that conifer.h was already put into cmssw in the ParticleFlow section https://github.com/cms-sw/cmssw/blob/3cd98a0b792252bb046157709fc9a3029f148cc2/L1Trigger/Phase2L1ParticleFlow/interface/conifer.h, can we use this file instead of adding another one into CMSSW?

Chriisbrown commented 1 year ago

Upon further investigation, I actually see that conifer.h was already put into cmssw in the ParticleFlow section https://github.com/cms-sw/cmssw/blob/3cd98a0b792252bb046157709fc9a3029f148cc2/L1Trigger/Phase2L1ParticleFlow/interface/conifer.h, can we use this file instead of adding another one into CMSSW?

I would imagine this isn't a permanent place for conifer in cmssw as it is in a specific place in the particle flow subfolder, maybe @thesps can comment on if there is a plan for conifer to be put in a more generic place in the future? If so I don't know if makes sense for us to point to the particle flow conifer if it is just going to be changed in the future?

Chriisbrown commented 1 year ago

Upon further investigation, I actually see that conifer.h was already put into cmssw in the ParticleFlow section https://github.com/cms-sw/cmssw/blob/3cd98a0b792252bb046157709fc9a3029f148cc2/L1Trigger/Phase2L1ParticleFlow/interface/conifer.h, can we use this file instead of adding another one into CMSSW?

I would imagine this isn't a permanent place for conifer in cmssw as it is in a specific place in the particle flow subfolder, maybe @thesps can comment on if there is a plan for conifer to be put in a more generic place in the future? If so I don't know if makes sense for us to point to the particle flow conifer if it is just going to be changed in the future?

So having spoken to Claire and Sioni on this we concluded that we should use the particle flow conifer file and delete our version, but be mindful that in the future this will change and we will need to point to an different conifer file. However, this poses a problem for this PR. The conifer file in particle flow has only just been introduced to cmssw (in CMSSW_13_2_X) so we can't access it while cms-l1tk is in CMSSW_12_5, so there are two solutions. Wait on this PR until our cmssw is rebased to CMSSW 13 (not sure what the time scale is on this) or modify our CI and cmssw build instructions to temporarily pull a single file from CMSSW_13_2_X until we have rebased to CMSSW 13. @tomalin thoughts?

tomalin commented 1 year ago

Upon further investigation, I actually see that conifer.h was already put into cmssw in the ParticleFlow section https://github.com/cms-sw/cmssw/blob/3cd98a0b792252bb046157709fc9a3029f148cc2/L1Trigger/Phase2L1ParticleFlow/interface/conifer.h, can we use this file instead of adding another one into CMSSW?

I would imagine this isn't a permanent place for conifer in cmssw as it is in a specific place in the particle flow subfolder, maybe @thesps can comment on if there is a plan for conifer to be put in a more generic place in the future? If so I don't know if makes sense for us to point to the particle flow conifer if it is just going to be changed in the future?

So having spoken to Claire and Sioni on this we concluded that we should use the particle flow conifer file and delete our version, but be mindful that in the future this will change and we will need to point to an different conifer file. However, this poses a problem for this PR. The conifer file in particle flow has only just been introduced to cmssw (in CMSSW_13_2_X) so we can't access it while cms-l1tk is in CMSSW_12_5, so there are two solutions. Wait on this PR until our cmssw is rebased to CMSSW 13 (not sure what the time scale is on this) or modify our CI and cmssw build instructions to temporarily pull a single file from CMSSW_13_2_X until we have rebased to CMSSW 13. @tomalin thoughts?

%%%% I've added this "to do" to the Track Quality issue https://github.com/cms-L1TK/cmssw/issues/222 , with @Chriisbrown and @cgsavard as the Issue's assignees. I believe we can therefore now merge this PR. I'll do it later today (UK time) unless anyone objects.