TST: Remove even more uses np.array_equal in tests
State: closed

Implement `assert_not` as a way to check that assertions should fail (for methods more sophisticated than a simple bare `assert`).  Also takes the opportunity to remove several more `np.array_equal` assertions.

Follow-up to #18047 

Labels:
name: Testing
description: pandas testing functions or related to the test suite

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=h1) Report
> Merging [#18087](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b4375bde87afdc037056ff90e4f93b606c76e140?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18087/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18087      +/-   ##
==========================================
- Coverage   91.27%   91.23%   -0.05%     
==========================================
  Files         163      163              
  Lines       50120    50120              
==========================================
- Hits        45749    45728      -21     
- Misses       4371     4392      +21
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.04% <ø> (-0.03%)` | :arrow_down: |
| #single | `40.32% <ø> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `100% <ø> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.38% <0%> (-1.82%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=footer). Last update [b4375bd...ac69b46](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=h1) Report
> Merging [#18087](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/148ed63f9287cc55f7a2802da300b717d01cabe6?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18087/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18087      +/-   ##
==========================================
- Coverage    91.4%   91.38%   -0.02%     
==========================================
  Files         164      164              
  Lines       49880    49880              
==========================================
- Hits        45592    45583       -9     
- Misses       4288     4297       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.19% <ø> (ø)` | :arrow_up: |
| #single | `39.42% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.8% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=footer). Last update [148ed63...85ba491](https://codecov.io/gh/pandas-dev/pandas/pull/18087?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

cc @jorisvandenbossche @TomAugspurger 
@jorisvandenbossche @TomAugspurger : Any thoughts?
Yeah, this seems good.
Is there a reason we can't use `assert not s.equals(s2)` ?
@jorisvandenbossche : Hmm...that's a good point.  How comprehensive is it vs. `tm.assert_series_equal` ?
In any case, I'm still in favor of using the method I have developed in cases where our equality checking isn't as strict (e.g. `check_dtype=False`)
`equals` only looks if the data are equal, not the attributes (eg not the Series name), so it less strict than `assert_series_equal`. 
But I don't think that this is a problem in this case. As because if a different name is the reason you want `assert_series_equal` to fail, you shouldn't use this, but just explicitly assert that the names are not equal.

So personally I don't find that an argument to have `assert_not(assert_series_equal(..))` (but there might be other arguments)
@jorisvandenbossche : Fair point.  This function is meant to be catch-all for any of our in-house `assert_*` functions, and initially it was meant to address our not having a counter-part for `assert_numpy_array_equal`.

That being said, `assert_not(assert_series_equal(...))` could indeed be simplified as you suggested, and I can make that change later today.
lgtm. @jorisvandenbossche ?
I am still not sure adding an `assert_not` to our own testing addons is worth it for the current single use case (and even for that one, I personally find `assert not np.array_equal(df.values, b_c)` much more natural to read than `tm.assert_not(tm.assert_numpy_array_equal, df.values, b_c)`)
@jorisvandenbossche : The whole point is not to use `np.array_equal` in the first place, as our testing methods with `assert_numpy_array_equal` are richer.  This was a more generalized solution rather than writing a one-off solution of `assert_not_numpy_array_equal`.

In any case, it just so happened that `Series` equality checking was sufficient in many of the cases where we were using `np.array_equal`, but this construct that I created can be used for those cases (albeit not so frequent) where we might want to check that a certain `assert` should fail.
@jorisvandenbossche : If you haven't done so already, have a look at #18047, where this whole discussion started around `array_equal`.
I didn't see that, but that issue does not seem to give a reasoning for the need to remove array_equal? 
And if that is the goal, I would use np.allclose
> I didn't see that, but that issue does not seem to give a reasoning for the need to remove array_equal?

I think the idea (and @jreback feel free to jump in) is that we were avoiding explicitly using the `numpy` API for our testing purposes and only using in-house comparisons during testing.  That's the emphasis that I have seen (and received feedback on over the course of previous PR's).

As written, `np.array_equal` works to get the tests passing, but we would rather check with a different function than that.
I am not sure if that is the general direction, eg with the move to pytest we actually moved a bit more away from our in-house testing functionality (and you did some of those PRs to replace in-house methods with standard pytest functionality). 
So I don't see why we are adding here new in-house functionality to maintain while there is a perfectly sensible solution with standard tools of numpy and pytest (unless there is a clear benefit of an in-house method, which is sometimes the case, but I don't see that here) 
I'm fine with incorporating `pytest` tools in-place of in-house.  However, `numpy` tools in-place of in-house, not so much.  More often than not, we can use something better than `array_equal` when testing, as you can see in this PR and the one preceding it.

So if you have a suggestion for writing this in `pytest` idiom, I'm all for it.  However, if we can't, then I'd rather do this in-house than rely on `numpy` tools.
> More often than not, we can use something better than array_equal when testing, as you can see in this PR and the one preceding it.

Can you then be more specific? What is 'bad' about array_equal in this specific use case?

> So if you have a suggestion for writing this in pytest idiom, I'm all for it. However, if we can't, then I'd rather do this in-house than rely on numpy tools.

I think `assert not np.array_equal(..)` or `assert not np.allclose(..)` is a perfect pytest idiom
@jorisvandenbossche : In this specific case, I'll give it to you that we actually don't need this paradigm at all, as the shape of the arrays are completely different (just ran through the test manually), making this `array_equal` check kind of ridiculous.

So in that case, no need to implement `assert_not` for this PR, and we can table that discussion for another time if need be.

That being said, `array_equal`, similar to `Series.equal` only checks values and shape, but not `dtype`, which is what `assert_numpy_array_equal` does.
> That being said, array_equal, similar to Series.equal only checks values and shape, but not dtype, which is what assert_numpy_array_equal does.

Yes, but as I said above, IMO if the reason that you want assert_series/numpy_array_equal to fail is not because the values differ but because the dtype (or name, ..) differs, I find `assert_not(assert_...())` very obscure and I think we should explicitly test that attribute.
So my reasoning: either the values differ -> simply use `.equals` / `array_equal`, or either the values are equal but the dtype, name, ... differ -> test that explicitly
@jorisvandenbossche : I just renamed the PR to just remove instances of "array_equal" from tests.  Everything is green, so PTAL.
thanks @gfyoung 
just for edification. we don't like ``np.array_equal`` because it doesn't work with NaNs; this is why we have ``array_equivalent``, which is you can use as well in tests.
> this is why we have array_equivalent

All this time, and I didn't know that existed. 😄  Should mention that from now on in case we have this situation down the road.
Thanks @gfyoung 

> we don't like np.array_equal because it doesn't work with NaNs; this is why we have array_equivalent, which is you can use as well in tests.

and on the numpy side, I think `allclose` is the 'replacement' for array_equal that also can handle NaNs