This repository has been archived by the owner on Jun 2, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding State Updating and # of Tasks to LSTM and RGCN models #104
Adding State Updating and # of Tasks to LSTM and RGCN models #104
Changes from 3 commits
94d0454
b9f5393
a1ff04f
b7e9046
494b931
b3f8df9
34e9983
10ffdbd
c97f179
cf95324
088d4a7
4101879
6fc687d
7c36a4f
c582d3d
ab4cff1
3129bd5
c8c140a
3e54465
e939460
17bfab8
2c7580b
3799509
a4d54e2
0f568ae
6cd7b52
9bc45ca
63bb515
8c80003
3eca6f2
4527d67
8d061f8
1c1641a
8362981
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter name suggests it'd be hard to generalize to other variables. Prod mostly to @jsadler2 to think about whether/how this parameter name and/or functionality should be adjusted to accommodate, say, DO predictions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved to the
model
level, not thelayer
level because we will be separating out the output layer from the RGCN layer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this variable be
W_out_task0
instead of specifying flow? and thenW_out_temp
could beW_out_task1
? Or the reverse, if that's how it goes - is temp task 1 or 2 in your conventions, @jsadler2 ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to add a docstring to this function so that we are clear on what the arguments are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining how lines 218-223 reshape
h_list
,c_list
, andout
(i.e., from what initial shapes to what final shapes)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again I think
num_tasks
would be slightly clearerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See renaming comment above. This could become something like
task0_in_task1
(or the reverse?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it that this wasn't being called before? And where does
self.rnn_layer
get used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider setting up a
states
property rather thanh_gr
andc_gr
properties, to be more similar to the LSTMstates
setupThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this idea, @jzwart? If we do this, you'd access it like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that would be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work to make these three lines a one-liner? If so, I think in a way that'd be clearer (b/c we don't have to wonder where else
h_gr
might get used).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change. I suggest renaming
rmse_main
tormse_1task
and renamingrmse_loss
tormse_2tasks
.