(Suspicion of) USE_DAB-inconsistencies

[johann-aoe]

Hey :)

First of all thanks for responding that quickly to my previous "Issue", I appreciate that you're still active here and maintaining the code so nicely. It's been a pleasure to work with your repository.

I have a brief question about something that I noticed, and wanted to clear up if I'm understanding something wrong or if there's a potential semantic inconsistency in your code: your TrackInstances-implementation defaults in the constructor to use_dab = False. This flag is being set to the config's value (e.g. "True") in the train_engine.py, so the created TrackInstance-instance has a query_embed of the size "hidden_dim" initially. However, this constructor-argument-default almost never gets overwritten by you in any of the followup TrackInstances instantiations, e.g. in the "to"-funciton of TrackInstances, the "getitem"-function, "cat_tracked_instances" etc. There you create new TrackInstances without passing the initial TrackInstance's use_dab-property.

So the code initially sets up (correctly) dab-positive TrackInstances, and then at e.g. a synchronization from CPU to GPU (the "to"-call) turns it into a dab-negative TrackInstance.

Is this intentionally?

I would appreciate an answer a lot, as I'm trying to understand your code as nicely as possible, and want to avoid any misconceptions of the code on my side.

Thanks in advance!

Johann

[HELLORPG]

Hey Johann,

Thank you for pointing out this issue. You are right—the use_dab flag was indeed forgotten in those functions. However, this oversight coincidentally doesn't actually affect the runtime correctness.
This parameter is only used to define the initial shape of some fields. In both to() and __getitem__(), after the new instance is initialized (using the wrong use_dab=False as default), all its fields are immediately overwritten by assignment statements. Because of this, they end up with the correct shape and content from the original instance, making the incorrect flag harmless.
That said, you're 100% right that it's semantically inconsistent and confusing. I'll push a fix to propagate the flag properly.

[johann-aoe]

Hey Hellorpg,

thanks for the quick response, again! :) Oh, you're right, for the to() and __getitem__(), that makes total sense. I fixed it on my local clone of the repository, and I interestingly still see round about 3% faster runtime, probably because it doesn't need to allocate the extra memory initially for the use_dab=False. Definitely not a lot, but I'll take those extra-minutes :P Just for completeness' sake, in query_updater.py, line 221, the same thing happens, but again in the upcoming lines it's being overwritten with the correct dimensions, so also there there's no practical difference, but might also be a candidate to add the "use_dab".

Thanks again for your response and great work!

Johann

[HELLORPG]

Got it! I'll also fix this line. Thank you a lot.