-
Notifications
You must be signed in to change notification settings - Fork 19.4k
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
Allow to reshape unknown tensors. #7960
Conversation
I believe the current design was due to previous limitations with TF. Tests are currently failing. Also, you should add a unit test demonstrating the limitation that is being lifted. |
0d994fe
to
281723f
Compare
keras/layers/core.py
Outdated
return (input_shape[0],) + self._fix_unknown_dimension( | ||
input_shape[1:], self.target_shape) | ||
if None in input_shape[1:]: | ||
return (input_shape[0],) + tuple(s if s != -1 else None for s in self.target_shape) |
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.
Please add a comment describing what this does / why do it, and make this line shorter.
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.
Done.
keras/layers/core.py
Outdated
if None in input_shape[1:]: | ||
return (input_shape[0],) + tuple(s if s != -1 else None for s in self.target_shape) | ||
else: | ||
return (input_shape[0],) + self._fix_unknown_dimension( |
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.
Since this function is only called once, consider in-lining it (not saying you should do it, but consider whether it would improve the code if you did it -- maybe it would).
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 had considered that, but I figured keeping it separate would be better. That way it can potentially be used by layers that subclass Reshape
(or other code in general). In addition it is more in line with the "separation of concerns" philosophy.
281723f
to
0c85f58
Compare
Updated PR as requested. |
0c85f58
to
509aca5
Compare
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.
LGTM, thanks
* Allow to reshape unknown tensors. * Add test for reshaping unknown shapes.
The current implementation of
keras.layers.Reshape
does not allow the reshaping of tensors of unknown shape (ie.(None, None, 3)
for any arbitrary image). Strangely enough, the underlying code oftensorflow.reshape
does allow this kind of reshape (and already handles-1
in the target shape), I'm not sure about other backends.Motivation
The following code shows what this PR fixes:
Without this PR the result is:
With this PR the result is:
ps. my current workaround is: