PERF: fix assert_frame_equal can be very slow by ivanovmg · Pull Request #38202 · pandas-dev/pandas

ivanovmg · 2020-12-01T04:23:13Z

closes BUG: assert_frame_equal can be very slow #38183
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Add kwarg check_index into assert_series_equal to allow elimination of multiple index checking (for each column) in assert_frame_equal.

Performance wise.

import pandas as pd
import numpy as np
from string import ascii_letters

idx = np.random.choice(np.array(list(ascii_letters)), size=10000).astype(object)
a = np.random.random_integers(0, 100, (10000, 1000))

%time pd.testing.assert_frame_equal(pd.DataFrame(a), pd.DataFrame(a.copy()))
Wall time: 319 ms

%time pd.testing.assert_index_equal(pd.Index(idx), pd.Index(idx.copy()))
Wall time: 993 µs

%time pd.testing.assert_frame_equal(pd.DataFrame(a, index=idx), pd.DataFrame(a, index=idx))
Wall time: 281 ms

ivanovmg · 2020-12-01T06:36:46Z

Looks like green.

jreback · 2020-12-01T11:45:29Z

hmm can you merge master (this looks like it's running some older code)

ivanovmg · 2020-12-01T16:00:59Z

Did that.
But presumably we have the same irrelevant failures anyway.

jreback · 2020-12-01T17:07:49Z

Did that.
But presumably we have the same irrelevant failures anyway.

it was some other errors.

but in spite of this change i am not sure this actually fixes things. (tests times are about the same)

jbrockmendel · 2020-12-01T17:21:21Z

tests times are about the same

i doubt we have any/many tests that are as poorly behaved as the example from #38183

jbrockmendel · 2020-12-01T17:22:34Z

pandas/_testing.py

                rtol=rtol,
                atol=atol,
+                check_index=False,
            )


we have a lot of kwargs already. would it be viable to call assert_(ea|numpy)_array_equal on lcol._values and rcol._values?

Probably.
But I looked inside assert_series_equal and noticed that there are a lot of cases covered, different dtypes, etc.
So, I thought that it would be safer to parametrize.
I will try to find another way, as you suggest.

@jbrockmendel, I am afraid I can only suggest the following approach for now.

Split assert_series_equal into assert_index_equal and _assert_series_values_equal.
The latter one is the same as the original assert_series_equal, but with the assert_index_equal removed from there.

Then we can use _assert_series_values_equal in the loop inside assert_frame_equal.

works for me

hmm i actually liked your prior implementation (check_index) but i guess this is fine.

ivanovmg · 2020-12-01T17:27:19Z

Did that.
But presumably we have the same irrelevant failures anyway.

it was some other errors.

but in spite of this change i am not sure this actually fixes things. (tests times are about the same)

Execution of the following command improved.

%time pd.testing.assert_frame_equal(pd.DataFrame(a, index=idx), pd.DataFrame(a, index=idx))

from 50 seconds to 280 something milliseconds (#38183).

ivanovmg · 2020-12-01T17:56:53Z

I can think of a test ensuring that assert_frame_equals calls assert_index_equal only once.
This is not a performance test, but for now we see that the problem with the performance observed in the original issue is due to multiple calls of the latter one.
What do you think of this approach?
@jreback @jbrockmendel

In fact, assert_index_equal is called twice: first time for checking index equivalence and second - for columns.
Added this test.

jbrockmendel · 2020-12-02T00:11:02Z

pandas/tests/util/test_assert_frame_equal.py

@@ -1,3 +1,5 @@
+from unittest.mock import patch


i think elsewhere we use pytest's monkeypatch fixture, does that not provide the call_count check?

I did not find this functionality there.

jbrockmendel · 2020-12-02T00:12:03Z

pandas/tests/util/test_assert_frame_equal.py

        tm.assert_frame_equal(left, right)
+
+
+def test_assert_frame_equal_checks_index_exactly_twice():


i dont think this necessarily needs a test, maybe an asv. as long as there is a comment in tm.assert_frame_equal about why we are doing what we're doing that should be enough

I am removing the test because it relies on unittest.mock, which is not accepted by the CI check.

jbrockmendel · 2020-12-02T00:13:41Z

pandas/_testing.py

@@ -1690,11 +1727,10 @@ def assert_frame_equal(
            assert col in right
            lcol = left.iloc[:, i]
            rcol = right.iloc[:, i]


marginally faster to do right._ixs(i, axis=1)

jbrockmendel · 2020-12-02T00:14:04Z

pandas/_testing.py

+    atol=1.0e-8,
+    obj="Series",
+):
    if check_less_precise is not no_default:


__tracebackhide__ = True

jreback · 2020-12-02T01:22:00Z

Did that.
But presumably we have the same irrelevant failures anyway.

it was some other errors.
but in spite of this change i am not sure this actually fixes things. (tests times are about the same)

Execution of the following command improved.
%time pd.testing.assert_frame_equal(pd.DataFrame(a, index=idx), pd.DataFrame(a, index=idx))
from 50 seconds to 280 something milliseconds (#38183).

ok maybe something else is stil take time

jreback · 2020-12-02T01:23:06Z

its this build: https://travis-ci.org/github/pandas-dev/pandas/jobs/747040921 that is really really slow, used to be like 30min.

jreback · 2020-12-02T01:23:18Z

pre-commit checks are faling

jreback · 2020-12-02T01:24:32Z

pandas/_testing.py

+
+
+def _assert_series_values_equal(
+    left,


yeah let's revert this and use the check_index=False

Reverted to use of check_index.

But personally I prefer the way involving splitting of assert_series_equal into two functions.
Indeed, this function does two major things: check index equivalence and check values equivalence, so it is reasonable to split it like that.
I see that there are multiple parameters passed to _assert_series_values_equal, which make this solution look not very elegant.
But there is a benefit that we do not touch public API, by not adding new parameter check_index.
Please suggest which solution you prefer (@jbrockmendel suggested that we do not introduce new kwarg).

the new kwarg isnt the worst thing in the world. lets make it keyword-only to start the process of #38222

ok i think this is fine (reason I don't mind check_index is that we have this elsewhere publicly). ok let's make this kwarg only fo r the new paramater, otherwise lgtm.

I make this kwarg keyword-only.

This reverts commit fd5dc65.

This reverts commit e8fe687.

simonjayhawkins · 2020-12-03T11:19:26Z

We normally have a high bar for adding parameters to the public api. Since assert_series_equal is public, should we give this more consideration.

we could just have an internal assert_series_equal for now with the extra parameter, and dispatch from the public assert_series_equal with the api unchanged.

jreback · 2020-12-07T13:45:22Z

We normally have a high bar for adding parameters to the public api. Since assert_series_equal is public, should we give this more consideration.

we could just have an internal assert_series_equal for now with the extra parameter, and dispatch from the public assert_series_equal with the api unchanged.

-1 on this (which was already suggested), this add even more complexity in an already complex world. we already have a check_index keyword so nbd here.

jreback · 2020-12-23T14:18:40Z

pandas/_testing.py

+    check_index : bool, default True
+        Whether to check index equivalence. If False, then compare only values.
+
+        .. versionadded:: 1.2.0


move to 1.3

jreback · 2020-12-23T14:19:08Z

pandas/_testing.py

            assert col in right
-            lcol = left.iloc[:, i]
-            rcol = right.iloc[:, i]
+            lcol = left._ixs(i, axis=1)


why are you changing this? this is not a public function and shouldn't be used here at all

@jbrockmendel suggested that it would be a marginally faster option.
Looks like it actually is.

it doesn't matter if its faster as its not public; these are heavily used public function (yes this is an implementation), but we want to use iloc here (and i doubt this actually makes any real world difference).

jreback · 2020-12-23T15:50:22Z

can you show how this is faster, specifically are things decreasing in the duration? which builds are faster?

ivanovmg · 2020-12-24T10:01:25Z

can you show how this is faster, specifically are things decreasing in the duration? which builds are faster?

I compared iloc vs _ixs and did not notice much difference. So, I reverted back to iloc.

jreback · 2020-12-24T20:40:45Z

thanks @ivanovmg

PERF: fix assert_frame_equal can be very slow

5970d41

ivanovmg requested review from jbrockmendel and jreback December 1, 2020 05:00

Merge branch 'master' into bug_38183

0be8f99

jbrockmendel reviewed Dec 1, 2020

View reviewed changes

ivanovmg force-pushed the bug_38183 branch from b9bfa96 to c698913 Compare December 1, 2020 17:48

REF: extract function _assert_series_values_equal

fd5dc65

ivanovmg force-pushed the bug_38183 branch from c698913 to fd5dc65 Compare December 1, 2020 17:50

TST: add test with mock

e8fe687

ivanovmg force-pushed the bug_38183 branch from e095cef to e8fe687 Compare December 1, 2020 18:09

jbrockmendel reviewed Dec 2, 2020

View reviewed changes

pandas/_testing.py

atol=1.0e-8,

obj="Series",

):

if check_less_precise is not no_default:

Copy link

Member

jbrockmendel Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__tracebackhide__ = True

jreback reviewed Dec 2, 2020

View reviewed changes

jreback added the Testing pandas testing functions or related to the test suite label Dec 2, 2020

ivanovmg added 3 commits December 2, 2020 11:05

Revert "REF: extract function _assert_series_values_equal"

fbfbcdd

This reverts commit fd5dc65.

Revert "TST: add test with mock"

f778a7d

This reverts commit e8fe687.

PERF: replace iloc with ixs

e0cf672

ivanovmg requested a review from jreback December 2, 2020 07:46

ivanovmg requested a review from jbrockmendel December 2, 2020 07:46

ivanovmg added 2 commits December 3, 2020 00:58

Merge branch 'master' into bug_38183

59a178d

REF: make check_index keyword-only

82cb7a3

jreback added this to the 1.2 milestone Dec 2, 2020

jreback removed this from the 1.2 milestone Dec 7, 2020

ivanovmg added 2 commits December 8, 2020 11:01

Merge branch 'master' into bug_38183

932ec28

Merge branch 'master' into bug_38183

bf18386

jreback requested changes Dec 23, 2020

View reviewed changes

ivanovmg added 2 commits December 24, 2020 16:59

REF: revert to iloc

59bc277

Merge branch 'master' into bug_38183

3401d8d

DOC: update versionadded to 1.3

62058bf

jreback added this to the 1.3 milestone Dec 24, 2020

jreback approved these changes Dec 24, 2020

View reviewed changes

jreback merged commit df90970 into pandas-dev:master Dec 24, 2020

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Dec 27, 2020

PERF: fix assert_frame_equal can be very slow (pandas-dev#38202)

0827cd0

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021

PERF: fix assert_frame_equal can be very slow (pandas-dev#38202)

1dd97e2

		tm.assert_frame_equal(left, right)


		def test_assert_frame_equal_checks_index_exactly_twice():

Uh oh!

Conversation

ivanovmg commented Dec 1, 2020

Uh oh!

ivanovmg commented Dec 1, 2020

Uh oh!

jreback commented Dec 1, 2020

Uh oh!

ivanovmg commented Dec 1, 2020

Uh oh!

jreback commented Dec 1, 2020

Uh oh!

jbrockmendel commented Dec 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanovmg commented Dec 1, 2020

Uh oh!

ivanovmg commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Dec 2, 2020

Uh oh!

jreback commented Dec 2, 2020

Uh oh!

jreback commented Dec 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanovmg Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonjayhawkins commented Dec 3, 2020

Uh oh!

jreback commented Dec 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Dec 23, 2020

Uh oh!

ivanovmg commented Dec 24, 2020

Uh oh!

jreback commented Dec 24, 2020

Uh oh!

Reviewers

ivanovmg commented Dec 1, 2020 •

edited

Loading

ivanovmg Dec 2, 2020 •

edited

Loading