Khrylx / AgentFormer

[ICCV 2021] Official PyTorch Implementation of "AgentFormer: Agent-Aware Transformers for Socio-Temporal Multi-Agent Forecasting".
https://www.ye-yuan.com/agentformer/
MIT License
257 stars 64 forks source link

possibly missing an else clause in preprocessor.py #29

Open arielchen07 opened 1 year ago

arielchen07 commented 1 year ago

https://github.com/Khrylx/AgentFormer/blob/e4fe8dd6df3b3d2033665c5f8ac3544e81c44db5/data/preprocessor.py#L67-L69

Hi, thanks for the great work done on AgentFormer!

While going through the model's code, I noticed that there might be a missing else clause in preprocessor.py, specifically on lines 67 to 69. I'm unsure if this is a typo or if I have misunderstood something.

I would appreciate if you can give some clarification on that. Thanks in advance!

PFery4 commented 1 year ago

From the looks of it, the conditional does not have any effect on the definition of the data variable, as it will directly be overwritten by the assignment made on line 69.

Here, data is a block of all rows extracted from the entire .txt file, which have their timestep value equal to frame - i * self.frame_skip

Perhaps the condition imposed in line 67 was initially meant to catch the eventual case where there simply is no data to be found for the provided input frame (ie, timestep). Effectively though, the check does not have any effect.

But what does happen then? That is, how does line 69 resolve when not a single row in self.gt validates the condition self.gt[:, 0] == (frame - i * self.frame_skip)? Well, the indexing will be performed without error, but the resulting array will be essentially empty (and of shape (0, 17), with 17 corresponding to the number of columns in the initial .txt file from which the data is extracted).

Having an empty data variable here is no big deal, as this is eventually "caught" later in the call method (line 163)

https://github.com/Khrylx/AgentFormer/blob/e4fe8dd6df3b3d2033665c5f8ac3544e81c44db5/data/preprocessor.py#L163-L164

So, essentially, lines 67 and 68 can be safely removed from the script here, this won't alter the behaviour of the script (and also save a bit of computation time as well)