-
Notifications
You must be signed in to change notification settings - Fork 349
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
support recurrent with no states. #1113
Open
Beronx86
wants to merge
7
commits into
mila-iqia:master
Choose a base branch
from
Beronx86:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2b45c97
support recurrent with no states.
Beronx86 2fdba23
recurrent wrapper without states test case
Beronx86 69b1846
recurrent wrapper without states test case
Beronx86 0d49671
make recurrent with no states easier. test case for nested scan_varia…
Beronx86 ba6f6ae
a test case for nested recurrent model. assert there is only one top_…
Beronx86 c5b015f
revert back the change in BaseRecurrent.initial_states
Beronx86 0de2dc7
remove recurrent_with_no_states test case, since the above change wou…
Beronx86 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
"""Annotated computation graph management.""" | ||
import logging | ||
from collections import OrderedDict | ||
from collections import OrderedDict, deque | ||
from itertools import chain | ||
import warnings | ||
|
||
|
@@ -103,8 +103,15 @@ def auxiliary_variables(self): | |
|
||
@property | ||
def scan_variables(self): | ||
"""Variables of Scan ops.""" | ||
return list(chain(*[g.variables for g in self._scan_graphs])) | ||
"""Variables of Scan ops. Breadth-first search""" | ||
sg_que = deque(self._scan_graphs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code supposed that no recurrent class is nested. #1115 |
||
var_list = [] | ||
while sg_que: | ||
g = sg_que.popleft() | ||
var_list.append(g.variables) | ||
if g._scan_graphs: | ||
sg_que.extend(g._scan_graphs) | ||
return list(chain(*var_list)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a desirable change, but I have a few concerns:
|
||
|
||
def _get_variables(self): | ||
"""Collect variables, updates and auxiliary variables. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 explain how it works? I cannot immediately see 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.
when some subclass call the default
initial_states
function in theBaseRecurrent
class. This line would check whether it is necessary to return the initial states. If the subclass does not have anapply
method or itsapply
method does not containstates
, theinitial_states
would not return anything.This line would make it to support
recurrent
class with noapply
function or with nostates
.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.
Why do you want to have a class without
apply
? It's a mistake if a user forgot to defineapply
and the best is to crash soon.In a case if
apply.states
is empty,initial_states
would return an empty list before this change, why is it wrong?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.
If this line is added, the above code, which contains a recurrent brick with no apply method, would run well.
But, you are right about the
apply
method. TheBrick
subclass should follow some design rules. The problem is no code checks whether there is anapply
method in aBrick
subclass at present.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.
@Beronx86 , checking
apply.states
inBaseRecurrent.initial_states
is not a solution. There are quite a few places in Blocks-dependent code whereinitial_states
method is overloaded. Instead, like in your previous solution,initial_states
should not be called ifapplication
does not havestates
. Can you please revert back to the previous version of your fix?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.
@rizar I think this check could be carried out in
Brick.__init__
method. So we can make sure allBrick
subclasses containapply
methods. I reverted back the changes inBaseRecurrent
.