-
Notifications
You must be signed in to change notification settings - Fork 73
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
Use SeqLSTM #46
Use SeqLSTM #46
Conversation
Looks great! And should make it easier if there are future improvements to LSTMs as they'll probably come from having a sequential unit. Given torch/nn#860, |
In |
Broken (see Element-Research/rnn#172). Irrelevant with #46.
@JoostvDoorn are we ready to merge this now (pending resolving any conflicts)? |
@Kaixhin I think so. I will clean this up, and prepare for merge. |
@JoostvDoorn Any updates? |
00df464
to
d4bf957
Compare
Sorry for the delay. I cleaned up the code, will be running some tests to see if it still works. |
The code as such will break async, it is probably fine if async uses FastLSTM and use SeqLSTM for recurrent. @Kaixhin what is your take on this? |
Seeing as recurrency was only supported for one-step async, I think it's fine to focus on the DQN for now. That said, eventually it would be good to get recurrent support for all async agents, even if they need different implementations. |
It should now work for recurrent DQN, and should not break async. Sorry for taking such a long time, I am not working with this code at the moment. |
I am rewriting some stuff to use SeqLSTM instead of FastLSTM with the recurrent option. This will make things faster, especially if people start using longer histories. Depends on torch/nn#861, and torch/nn#889.