search

cms-L1TK / firmware-hls

HLS implementation of the tracklet pattern reco modules of the Hybrid tracking chain
15 stars 24 forks source link

Migration from iMath to plain HLS #314

Closed aryd closed 8 months ago

aryd commented 9 months ago

The main change in this PR is to migrate from using the iMath code for the calculation of the tracklet parameters and projections to using plain HLS. There is also some simplification of LUTs used in the tracklet parameter calculations and the projections.

aehart commented 9 months ago

@aryd This new set of test vectors causes inconsistencies in the TrackBuilder: https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/35417954

In particular, it looks like the HLS sometimes writes out extra tracks compared to the emulation. Any idea why that would be?

aryd commented 9 months ago

Andrew,

I don’t know. It is OK for the other processing steps? I can’t think of something that would in particular affect the TB. (Over the weekend I was looking at a problem where I get extra matches in the emulation, but this is after additional changes that are not in any PR.) I can take a look, maybe later today, but I have to prepare for my teaching tomorrow…

-Anders

Anders Ryd @.**@.>

On Jan 29, 2024, at 7:29 AM, Andrew Hart @.**@.>> wrote:

@arydhttps://github.com/aryd This new set of test vectors causes inconsistencies in the TrackBuilder: https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/35417954

In particular, it looks like the HLS sometimes writes out extra tracks compared to the emulation. Any idea why that would be?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/firmware-hls/pull/314#issuecomment-1914596312, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTIBPC6UQIOZXJXMZWDYQ6I2HAVCNFSM6AAAAABCEDL6OKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJUGU4TMMZRGI. You are receiving this because you were mentioned.Message ID: @.***>

aryd commented 8 months ago

Andrew, I don’t know. It is OK for the other processing steps? I can’t think of something that would in particular affect the TB. (Over the weekend I was looking at a problem where I get extra matches in the emulation, but this is after additional changes that are not in any PR.) I can take a look, maybe later today, but I have to prepare for my teaching tomorrow… -Anders Anders Ryd @.**@.> On Jan 29, 2024, at 7:29 AM, Andrew Hart @.**@.>> wrote: @arydhttps://github.com/aryd This new set of test vectors causes inconsistencies in the TrackBuilder: https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/35417954 In particular, it looks like the HLS sometimes writes out extra tracks compared to the emulation. Any idea why that would be? — Reply to this email directly, view it on GitHub<#314 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTIBPC6UQIOZXJXMZWDYQ6I2HAVCNFSM6AAAAABCEDL6OKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJUGU4TMMZRGI. You are receiving this because you were mentioned.Message ID: @.***>

There was a problem with how the test vectors were generated. I had not properly set doMultipleMatches to false. This led to errors in the MP and TB. I have pushed a fix to this problem. TB seems to be fine - but there are a few errors in the MP. We probably need to ask Brent to look into this. But I think we can live with a few errors. (The errors in the MP should not be related to the changes in the TP to remove iMath - I think.)

aehart commented 8 months ago

@aryd I agree we should merge this while the errors in the MP are investigated. My only comment besides the above inline ones is that there are several magic numbers in TrackletLUTs.h related to the widths and depths of the LUTs. Ideally, these should be replaced by named constants.