Remove support for *args in pandas wrappers #299

casperdcl · 2016-10-29T16:26:24Z

Fix total computation for pandas apply

migrated from #244, and rebased

casperdcl · 2016-10-29T16:31:21Z

@aplavin @CrazyPython not sure what you wanted to achieve with this. seems like once all the commits from #244 were squashed there's just the one change (drop support of *args). did i miss something?

codecov-io · 2016-10-29T16:34:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a379e33). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #299   +/-   ##
=========================================
  Coverage          ?   91.02%           
=========================================
  Files             ?        7           
  Lines             ?      546           
  Branches          ?      100           
=========================================
  Hits              ?      497           
  Misses            ?       48           
  Partials          ?        1

lrq3000 · 2016-10-29T21:16:57Z

tqdm/_tqdm.py

+                        axis = kwargs.get('axis', 0)
+                        total = df.shape[axis]
+                        if isinstance(df, DataFrame):
+                            total += 1  # pandas calls update once too many


@casperdcl Look at this part of the code, this is to fix applymap and other commands that provide a wrong len(df), in these cases it's necessary to use shape[axis] but the axis is difficult to get: it can either be a positional argument or a named argument. The problem with the positional argument is that depending on the pandas method, it's not the same position (normally it should be args[0] but for example apply()'s first argument is a boolean, not the axis, so it would fail.

My suggestion was to drop args altogether but output a tqdm warning if it detects a call with positional arguments, and to keep Aplavin's length detection code because it might work better (but i didn't try on everything, I just ran the unit test. Maybe we should add a new unit test for this, to test length detection for every kind of methods and scenario?).

Also you can read the hidden discussions in #244 (because they are on old commits).

So to be concise: TODO:

Add tqdm warning message if len(args) > 0

Maybe add unit tests for series and dataframe length detection (according to axis and method used).

I think we need to get someone who develops similar things in pandas to take a look. Don't really have the time to investigate seeing as I don't even use this feature.

lrq3000 · 2016-10-29T22:15:11Z

Ok I agree, then let's keep this PR open until we get someone competent to
review and contribute to this.

2016-10-29 23:36 GMT+02:00 Casper da Costa-Luis notifications@github.com:

@casperdcl commented on this pull request.

In tqdm/_tqdm.py #299:
             # Precompute total iterations
             total = getattr(df, 'ngroups', None)
             if total is None:  # not grouped
               total = len(df) if isinstance(df, Series) \
                   else df.size // len(df)
               if df_function == 'applymap':
                   total = df.size
               else:
                   axis = kwargs.get('axis', 0)
                   total = df.shape[axis]
                   if isinstance(df, DataFrame):
                       total += 1  # pandas calls update once too many
I think we need to get someone who develops similar things in pandas to
take a look. Don't really have the time to investigate seeing as I don't
even use this feature.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#299, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABES3pDRAhMknflKJztHAt2xg0YwxD-Gks5q47xfgaJpZM4KkJ18
.

CrazyPython · 2016-10-29T22:19:23Z

@casperdcl huh?

lrq3000 · 2016-10-29T23:01:46Z

Casper does not frequently use pandas, he already said so. He just
implemented the tqdm_pandas submodule using only his great insights into
the pandas API. But we are reaching a state where we need someone that is
more expert than us in pandas. I used pandas for a whole project in the
past, so you can say that I am a casual user, and this is getting too
complex for me too.

2016-10-30 0:19 GMT+02:00 CrazyPython notifications@github.com:

@casperdcl https://github.com/casperdcl huh?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#299 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABES3h9dQ9LditGk54sqxePJ52a2s7oIks5q48ZsgaJpZM4KkJ18
.

CrazyPython · 2016-10-30T22:38:40Z

add help-wanted.

@aplavin review this

aplavin · 2016-10-30T22:53:00Z

@CrazyPython well, this is my patch - how can I review it? :) To best of my knowledge it's correct, but I'm not a pandas developer either.
I myself use tqdm with this modification and it works ok.

lrq3000 · 2016-10-30T22:55:07Z

Thank's @aplavin. Well, then we either should just merge it in and see if it fixes issues (or if someone reports incorrect behavior), or add additional unit tests (to check correct length detection on various pandas datatypes and axis argument, if that's possible).

casperdcl · 2016-11-12T16:21:19Z

this fails unit tests on my machine FAIL: Test pandas.DataFrame[.series].progress_apply in tests_pandas.py line 60:

AssertionError: 
Expected:
100% at least twice

lrq3000 · 2016-11-12T16:25:37Z

Ok I'll have a look, the tests passed last time I tried.

2016-11-12 17:21 GMT+01:00 Casper da Costa-Luis notifications@github.com:

this fails unit tests on my machine FAIL: Test pandas.DataFrame[.series].
progress_apply in tests_pandas.py line 60:

AssertionError:
Expected:
100% at least twice

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#299 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABES3s5w1KFok3ssO8118s9k_R3WxDtkks5q9eeAgaJpZM4KkJ18
.

lrq3000 · 2016-11-14T13:46:52Z

@casperdcl Ah sorry I confused this PR with #271. I have no idea why this fails, but probably because of the different axis handling. We should put this PR aside until someone with more experience can provide a better fix...

casperdcl · 2016-12-10T11:13:23Z

from #322:

# Precompute total iterations
if kwargs.get('axis', 0)==1:
    total = len(df)
        else:
            total = getattr(df, 'ngroups', None)
                if total is None:  # not grouped
                    total = len(df) if isinstance(df, Series) \
                        else df.size // len(df)
                else:
                    total += 1  # pandas calls update once too many

lrq3000

I think we should merge this (see #322). We will tweak this later when someone with more knowledge come by (at least we will get relevant issues!).

Fix total computation for pandas apply

casperdcl · 2017-05-01T17:59:49Z

Just rebased onto master. But CI now failing.

cancan101 · 2017-07-12T04:22:02Z

any updates on this?

casperdcl · 2017-07-15T20:54:06Z

well if anyone (@cancan101?) would like to fix the conflicts and unit tests, we'd be happy to merge this in...

Raj-JainHC · 2017-11-06T12:48:11Z

@"People with write access", can anyone please review and merge the changes. 🙏

chengs · 2018-03-09T12:29:20Z

I am using pandas frequently. Maybe I could help.

casperdcl · 2018-03-10T14:09:00Z

thanks @chengs

chengs · 2018-03-14T16:34:08Z

even if we exclude args, still there is a problem to get a correct "total".

pandas apply calls func twice on the first column/row to decide whether it can take a fast or slow code path.

so, sometimes apply_func will run one more time, but maybe not if the func is slow.

still trying to find a way to determine if total = total + 1 or not.

chengs · 2018-03-14T17:57:41Z

please check #524 @casperdcl @lrq3000
I cannot commit to this #299

casperdcl mentioned this pull request Oct 29, 2016

Fix total computation for pandas apply #244

Closed

casperdcl added p0-bug-critical ☢ Exception rasing to-review 🔍 Awaiting final confirmation labels Oct 29, 2016

lrq3000 reviewed Oct 29, 2016

View reviewed changes

lrq3000 added the help wanted 🙏 We need you (discussion or implementation) label Oct 30, 2016

casperdcl force-pushed the master branch 4 times, most recently from 8cade97 to a65e347 Compare October 31, 2016 02:34

casperdcl force-pushed the aplavin-patch-2 branch from fe4a59a to b45e5b4 Compare November 12, 2016 17:00

casperdcl added this to the v5.0.0 milestone Nov 13, 2016

lrq3000 added to-fix ⌛ In progress and removed to-review 🔍 Awaiting final confirmation labels Nov 14, 2016

lrq3000 removed this from the v5.0.0 milestone Nov 14, 2016

casperdcl force-pushed the master branch from c9cc5ab to 5faf18b Compare December 5, 2016 01:06

casperdcl added p2-bug-warning ⚠ Visual output bad and removed p0-bug-critical ☢ Exception rasing labels Dec 6, 2016

casperdcl mentioned this pull request Dec 10, 2016

Pandas progress bar when iterating over rows #322

Closed

lrq3000 approved these changes Dec 27, 2016

View reviewed changes

kpj mentioned this pull request Feb 19, 2017

Missing total for DataFrame.progress_apply #351

Closed

interrogator mentioned this pull request Apr 4, 2017

Pandas apply on either axis #366

Closed

aplavin and others added 2 commits May 1, 2017 18:03

Remove support for *args in pandas wrappers

043b25f

Fix total computation for pandas apply

grammar

83feeb8

casperdcl force-pushed the aplavin-patch-2 branch from b45e5b4 to 83feeb8 Compare May 1, 2017 17:04

casperdcl assigned casperdcl and unassigned casperdcl May 5, 2017

cancan101 mentioned this pull request Jul 12, 2017

set total for apply on axis 1 #409

Closed

3 tasks

casperdcl force-pushed the master branch 6 times, most recently from 6ec00f1 to 4b6476a Compare July 22, 2017 14:15

chengs mentioned this pull request Mar 14, 2018

fix the problem #299 in terms of using tqdm in pandas #521

Closed

chengs mentioned this pull request Mar 17, 2018

fix the problem #299 in terms of using tqdm in pandas #524

Merged

3 tasks

casperdcl closed this Apr 3, 2018

casperdcl deleted the aplavin-patch-2 branch April 3, 2018 09:52

Uh oh!

Remove support for *args in pandas wrappers #299

Remove support for *args in pandas wrappers #299

Uh oh!

Conversation

casperdcl commented Oct 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperdcl commented Oct 29, 2016

Uh oh!

codecov-io commented Oct 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lrq3000 Oct 29, 2016

Choose a reason for hiding this comment

Uh oh!

lrq3000 Oct 29, 2016 • 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!

casperdcl Oct 29, 2016

Choose a reason for hiding this comment

Uh oh!

lrq3000 commented Oct 29, 2016

@casperdcl commented on this pull request.

Uh oh!

CrazyPython commented Oct 29, 2016

Uh oh!

lrq3000 commented Oct 29, 2016

Uh oh!

CrazyPython commented Oct 30, 2016

Uh oh!

aplavin commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrq3000 commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperdcl commented Nov 12, 2016

Uh oh!

lrq3000 commented Nov 12, 2016

Uh oh!

lrq3000 commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperdcl commented Dec 10, 2016

Uh oh!

lrq3000 left a comment

Choose a reason for hiding this comment

Uh oh!

casperdcl commented May 1, 2017

Uh oh!

cancan101 commented Jul 12, 2017

Uh oh!

casperdcl commented Jul 15, 2017

Uh oh!

Raj-JainHC commented Nov 6, 2017

Uh oh!

chengs commented Mar 9, 2018

Uh oh!

casperdcl commented Mar 10, 2018

Uh oh!

chengs commented Mar 14, 2018

Uh oh!

chengs commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

casperdcl commented Oct 29, 2016 •

edited

Loading

codecov-io commented Oct 29, 2016 •

edited

Loading

lrq3000 Oct 29, 2016 •

edited

Loading

aplavin commented Oct 30, 2016 •

edited

Loading

lrq3000 commented Oct 30, 2016 •

edited

Loading

lrq3000 commented Nov 14, 2016 •

edited

Loading

chengs commented Mar 14, 2018 •

edited

Loading