Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequence length #9

Closed
maria8899 opened this issue Jun 29, 2021 · 7 comments
Closed

Sequence length #9

maria8899 opened this issue Jun 29, 2021 · 7 comments

Comments

@maria8899
Copy link

Hi
Thanks for opening your code, very useful.
I noticed you implemented a seq_len parameter but you don't use it in the code or in your paper as I think all the models are single input image. I was wondering if you have played with this parameter with a sequence >1 and if you saw any difference?

@ap229997
Copy link
Collaborator

All the models in the paper have single time-step input. Our initial experiments with seq_len > 1 did not show any improvement so we did not include them in the paper.

The code is written in such a way that you can just change seq_len in the config and everything should run properly so you can also experiment more with this parameter.

@maria8899
Copy link
Author

I see, thank you for your explanations

@xinshuoweng
Copy link

Hey @ap229997, I understand your observation, but any intuition why using longer history does not help improve performance? Thanks in advance!

@ap229997
Copy link
Collaborator

Some previous works have reported that using observation histories could lead to causal confusion issue since expert actions are strongly correlated over time.

I still think that observation history should be beneficial and we probably didn't do a good enough job of investigating it thoroughly.

@xinshuoweng
Copy link

xinshuoweng commented Mar 17, 2022

Yeah, this is true! Thanks for pointing this out! Just out of curiosity, have you ever tried something simple? For example, in the current code implementation, it seems that the sequence of image/lidar features is just summed up in the pre_len dimension as shown below:

fused_features = torch.cat([image_features, lidar_features], dim=1)
fused_features = torch.sum(fused_features, dim=1)

Would it be more reasonable but also very simple to apply an LSTM here? Does changing to LSTM lead to worse performance since this is the most straightforward thing but NOT what's currently implemented?

BTW, this is Xinshuo Weng from Carnegie Mellon University. Thanks for sharing your experience here about this work, really appreciated it!! Also, very nice series of work from you on the Carla driving challenge!!

@ap229997
Copy link
Collaborator

We did not try LSTM and I agree that is an interesting design choice. The LiDAR point cloud is registered to the coordinate frame at the current timestep so I think even a simple summation should be ok. For the image input, the temporal aspect is not accounted for and LSTM could definitely be beneficial. Ideally, I would consider a temporal feature aggregation similar to FIERY.

Thanks for the kind words and your interest in our work.

@xinshuoweng
Copy link

Yeah, exactly, FIERY would be more ideal. Thanks a lot for that clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants