Updated test cases for reshape#2127
Conversation
* Increased the number of test cases from 5 to 7.
* The new tests cover the 0 dim case
* Changed the existing cases so that, where possible, the 'batch' dim is unchanged.
* Some frameworks do not allow for the first dim to change, so this makes these tests more generally applicable.
* Generated the new test data, updated the operators doc, and updated the test coverage doc.
|
Hmmm, I can't seem to figure out why travis-ci is failing. There doesn't seem to be a reason given. Just |
|
@linkerzhang any idea why the travis-ci build is failing here? Also, the circlecle build has started to fail but I don't think it is a problem on my side since it used to pass and I haven't made any changes since it last passed. The error message suggests a problem with the venv: |
|
|
|
@JamesAllingham I think Travis CI is failing, because your PR ends up modifying these files: These files are generated from other sources. |
|
@postrational Thanks, re-generating the files has fixed the issue. |
| 'reordered_dims': np.array([4, 2, 3], dtype=np.int64), | ||
| 'reduced_dims': np.array([3, 8], dtype=np.int64), | ||
| 'extended_dims': np.array([3, 2, 2, 2], dtype=np.int64), | ||
| 'reordered_dims': np.array([2, 4, 3], dtype=np.int64), |
There was a problem hiding this comment.
It seems that we should still have a test to test if the first dimension can be changed correctly. It doesn't sound right if we change all tests not to touch the first dimension. Those are tests for ONNX operators, not specific framework. What do you think?
There was a problem hiding this comment.
Totally agree, I did try to make sure that there were still tests touching the first dim, but I didn't think to specifically test reordering of the first dim with another. I'll add one back in.
| zero_index = np.where(shape == 0) | ||
| new_shape[zero_index] = np.array(original_shape)[zero_index] | ||
|
|
||
| reshaped = np.reshape(data, new_shape) |
There was a problem hiding this comment.
It means that we should have a reference implementation of ONNX Reshape. I am thinking something like
reshaped = onnx_reshape_impl(…)
There was a problem hiding this comment.
Sure, I'll do that 👍
wschin
left a comment
There was a problem hiding this comment.
Some places don't look right. Please take a look at my comments. Thanks.
…ing the first dim of a tensor
* Revamped test cases for reshape
* Increased the number of test cases from 5 to 7.
* The new tests cover the 0 dim case
* Changed the existing cases so that, where possible, the 'batch' dim is unchanged.
* Some frameworks do not allow for the first dim to change, so this makes these tests more generally applicable.
* Generated the new test data, updated the operators doc, and updated the test coverage doc.
* Re-generated Operators.md and TestCoverage.md
* Added a reshape op reference implementation and more tests for reshaping the first dim of a tensor
Increased the number of test cases from 5 to 7.
Changed the existing cases so that, where possible, the 'batch' dim is unchanged.
Generated the new test data, updated the operators doc, and updated the test coverage doc.