BUG: revert assert_numpy_array_equal to before c2ea8fb2 (accept non-ndarrays)

[toobaz]

• tests added / passed
• passes git diff upstream/master | flake8 --diff

assert_numpy_array_equal's docstring is at odds with (and the existence of the obj argument clashes with) the fact that it's now strict on its arguments being ndrarrays.

There are practical reasons to allow other kinds of arguments.

On the other hand, I don't see any reason for being strict, but I assume @sinhrks 's PR had some rationale behind: so a possible solution is an argument ensure_ndarray=True which restores the previous behaviour when set to False.

[codecov-io]

Current coverage is 84.23%

Merging #13355 into master will increase coverage by <.01%

@@ master #13355 diff @@ ========================================== Files 138 138 Lines 50721 50725 +4 Methods 0 0 Messages 0 0 Branches 0 0 ========================================== + Hits 42723 42727 +4  Misses 7998 7998 Partials 0 0 

Powered by Codecov. Last updated by 0c6226c...53ad5b1

[sinhrks]

-1 to allow non-ndarray only for a single issue. How about add check_dtype arg?

[jreback]

you can't simply change the test procedure. The entire point is to make tests more strick. So pls change as to @sinhrks suggestion.

Ideally we would be extremely strict on everything. The better checking that we have on tests the less ambiguity and the less that slips thru. The original PR did a lot to correct this.

[toobaz]

We want strict tests, this is a function. Which I'm not "changing", except for noticing that recent changes made its behaviour incoherent with docstring and arguments.

So leaving aside general lectures on fundamentals of computation, @sinhrks what's your suggestion? There already is a check_dtype arg... are you saying check_dtype=False should mean "don't even verify whether it's a numpy array"?

(fine for me, but we loose the possibility of comparing numpy arrays of different dtypes by still ensuring they are numpy arrays)

[sinhrks]

Sorry, forget about check_dtype. I may not understand original discussion properly, can you show a test case which should be succeed in this case?

[toobaz]

So, 4 options:

1. the current behaviour:

assert_numpy_array_equal(array([1,2]), [1, 2], check_dtype=WHATEVER) #Fails assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=False) #Passes assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=True) #Fails 

2. the previous behaviour/current docstring/current PR:

assert_numpy_array_equal(array([1,2]), [1, 2], check_dtype=WHATEVER) #Passes assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=False) #Passes assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=True) #Fails 

3. my proposal (new argument check_ndarray, presumably defaulting to True):

assert_numpy_array_equal(array([1,2]), [1, 2], check_ndarray=False, check_dtype=WHATEVER) #Passes assert_numpy_array_equal(array([1,2]), [1, 2], check_ndarray=True, check_dtype=WHATEVER) #Fails assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_ndarray=WHATEVER, check_dtype=False) #Passes assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_ndarray=WHATEVER, check_dtype=True) #Fails 

4. my interpretation of @sinhrks 's suggestion (making the existing check_dtype, which defaults to True, more strict):

assert_numpy_array_equal(array([1,2]), [1, 2], check_dtype=False) #Passes assert_numpy_array_equal(array([1,2]), [1, 2], check_dtype=True) #Fails assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=False) #Passes assert_numpy_array_equal(array([1,2]), array([1.0, 2]), check_dtype=True) #Fails 

If we stick with 1), it probably makes more sense to write a new function assert_is_copy, which would work on non-ndarrays too (as I was previously doing in my other PR), than to add the check_same argument to assert_numpy_array_equal as I am doing now.

I personally don't have strong preferences, we should just be coherent.

[sinhrks]

Closed by #13025, thx:)

[toobaz]

@sinhrks : this is maybe closed by you implicitly stating "we like the current behaviour" (option 1), but certainly not by #13205 (see this comment ) ;-) Which is fine by the way, if this is what you intended.

[sinhrks]

Ah I misunderstood #13025 covers curent needs.

Personally I'm +1 for option 1, but reopen for feedback from others.

[jreback]

yeah I think option 1 is fine, which is current behavior I think. @toobaz the cases you had worked around in #13205 look ok to me. If you find additional issues or think API needs changing, pls make a new issue (or we can discuss here).