This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
Fix infer shape partial after unknown shape changed to -1 #14869
Conversation
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
|
@mxnet-label-bot add [pr-work-in-progress] |
roywei
changed the title
|
@reminisce @haojin2 please help take a look, thanks! |
|
@mxnet-label-bot update[pr-awaiting-review] |
marcoabreu
added
pr-awaiting-review
haojin2
reviewed
May 16, 2019
haojin2
approved these changes
May 16, 2019
|
@reminisce Any more comments on this? |
reminisce
reviewed
May 18, 2019
src/operator/tensor/dot-inl.h
Outdated
| @@ -1207,6 +1207,14 @@ inline bool DotShape(const nnvm::NodeAttrs& attrs, | |||
| CHECK_EQ(out_attrs->size(), 1U); | |||
| mxnet::TShape& lshape = (*in_attrs)[0]; | |||
| mxnet::TShape& rshape = (*in_attrs)[1]; | |||
| // check if lhs ndim is larger than 1 and last dim is known | |||
| if (lshape.ndim() < 1 || !dim_size_is_known(lshape, lshape.ndim() - 1)) { | |||
There was a problem hiding this comment.
ndim=0 is a valid case representing scalar tensors. This line should be replaced by the following.
if (!ndim_is_known(lshape) || !ndim_is_known(rshape)) return false;
CHECK_GT(lshape.ndim(), 0) << "scalar tensor is not supported by this operator.";
CHECK_GT(rshape.ndim(), 0) << "scalar tensor is not supported by this operator.";
src/operator/tensor/dot-inl.h
Outdated
| return false; | ||
| } | ||
| // check if rhs ndim is larger than 1 and first dim is known | ||
| if (rshape.ndim() < 1 || !dim_size_is_known(rshape, 0)) { |
| @@ -8369,6 +8369,89 @@ def test_add_n(): | |||
| assert_almost_equal(rslt.asnumpy(), add_n_rslt.asnumpy(), atol=1e-5) | |||
|
|
|||
|
|
|||
| def test_dot_partial_shape(): | |||
There was a problem hiding this comment.
Please move shape inference tests to test_infer_shape.py.
reminisce
suggested changes
May 21, 2019
| assert result == [(-1, 3)] | ||
|
|
||
|
|
||
| def test_where_partial_shape(): |
There was a problem hiding this comment.
I think you forgot to call these test functions in __main__.
reminisce
approved these changes
May 21, 2019
This was referenced May 21, 2019
haohuanw
pushed a commit
to haohuanw/incubator-mxnet
that referenced
this pull request
Jun 23, 2019
* change check and shape_is_known * rever some changes * revert * revert * revert * add test * add more tests * update test dot * fix test * update reduce axes * fix lint * update check * fix lint * address comments * remove invalid test case * run all tests * update test case
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
fix #14833
As we changed unknown shape in mxnet from 0 to -1, some operators infer shape logic could be wrong.
MXNet unit tests are not testing for this corner case, but Keras-MXNet relies heavily on parietal shape infer, so many unit tests are failing.
changed
CHECK_GEandCHECK_LEto comparendim()with signed int as LHS could be -1 now.For some operators. when a tensor shape is entirely unknown, we should return directly. But we need to use
ndim_is_knowninstead ofshape_is_known. Because we can still continue to infer shape if tensor shape if partially unknown. Usingshape_is_knownwill cause a direct return unless the tensor shape is fully known.For binary ops, if LHS and RHS both have fully unknown shapes, we should return directly.
dotops is missing this logic.added unit tests
This fixes failing keras-mxnet unit tests.
Thanks to @reminisce for helping me debug and figure out the changes needed!
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Comments