added more tests to verify support for large vector #16477
Conversation
access2rohit
changed the title
|
@mxnet-label-bot add [pr-awaiting-review] |
|
@anirudh2290 @ChaiBapchya @zheng-da this PR is ready for review |
|
test_large_vector.test_slice ... ok |
|
randint not failing now ? |
|
Randint has been flaky. |
tests/nightly/test_large_vector.py
Outdated
| def test_ceil(): | ||
| x = create_input_for_rounding_ops() | ||
| y = nd.ceil(x) | ||
| assert y[LARGE_X//2-2] == -1 |
There was a problem hiding this comment.
what is this testing, can you add some comment here.
tests/nightly/test_large_vector.py
Outdated
| assert y[LARGE_X//2-2] == -1 | ||
| assert y[LARGE_X//2-1] == 0 | ||
| assert y[LARGE_X//2] == 0 | ||
| assert y[LARGE_X//2+1] == 0 |
There was a problem hiding this comment.
looks like a bunch of repeated code that can be reused in these 5 tests.
There was a problem hiding this comment.
all different assertions that explain the difference between each of the rounding functions
There was a problem hiding this comment.
this is a nice to have, not blocking on this. But this pattern can be refactored: create input, execute op, check output at specific indexes ( which happen to be same among all tests).
tests/nightly/test_large_vector.py
Outdated
| def test_fix(): | ||
| x = create_input_for_rounding_ops() | ||
| y = nd.fix(x) | ||
| assert y[LARGE_X//2-2] == -1 |
There was a problem hiding this comment.
can you add some comment of what it is testing, here and for other tests.
There was a problem hiding this comment.
aren't docs supposed to explain what a particular function is supposed to be doing? http://mxnet.incubator.apache.org/api/python/docs/api/ndarray/ndarray.html#mxnet.ndarray.fix
There was a problem hiding this comment.
my request is to explain the tests: why only these specific positions are checked as -1 or 0 , something to say x was this before we applied fix on it and what we get is expected because of this.
There was a problem hiding this comment.
ok got it. I will add comments explaining this. Good point
tests/nightly/test_large_vector.py
Outdated
| x = nd.array([0, 90, 180, 270, 360]) | ||
| x = nd.tile(x, LARGE_X//5) | ||
| y = nd.radians(x) | ||
| assert_almost_equal(y[0].asnumpy(), 0, atol=1e-3) |
There was a problem hiding this comment.
dont both parameters have to be numpy ndarrays ?
There was a problem hiding this comment.
yes.... this implementation worked too I can change it to following instead
assert np.abs(y[0].asnumpy() - 0) <= 1e-3
do you think this is better ?
There was a problem hiding this comment.
its fine if it works. maybe broadcasting by default
|
@wuxun-zhang maybe take a look for new tests :) |
access2rohit
force-pushed
the
new_large_vector_tests
branch
from
ae81eee
to
cb15ce3
Compare
tests/nightly/test_large_vector.py
Outdated
| expected_output = [-1, -1, 0, 0, 1] | ||
| assert_correctness_of_rounding_ops(y, LARGE_X//2, expected_output) | ||
|
|
||
| def test_rint(): |
There was a problem hiding this comment.
i have seen that internal functions starting with test_ can cause issues with nosetests, can you change the name to check_
There was a problem hiding this comment.
can you elaborate what kind of issues did you see ? Is there a previous github issue that I can look at, in order to understand why is it necessary to not have internal function names not starting with test_
I ran the the individual test and didn't see any issue:
test_large_vector.test_trigonometric_ops ... ok test_large_vector.test_rounding_ops ... ok
There was a problem hiding this comment.
nosetests generally looks for functions starting with "test_", thus this function could be mistaken for being a standalone test
access2rohit
force-pushed
the
new_large_vector_tests
branch
from
cb15ce3
to
00c8f05
Compare
|
|
|
test_large_vector.test_slice ... ok Ran 77 tests in 3950.523s OK |
|
@mxnet-label-bot add [pr-awaiting-merge] |
Description
Added large vector support for following ops:
astype
cast
repeat
ceil
fix
floor
rint
round
trunk
arccos
arcsin
arctan
cos
degrees
radians
sin
tan
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Testing
Currently running full suite of tests. Will update description later.