Fix total computation for pandas apply #244
Conversation
| else df.size // len(df) | ||
| if isinstance(df, Series): | ||
| total = len(df) | ||
| elif kwargs.get('axis', 0) == 0: |
There was a problem hiding this comment.
axis may also be in args instead of kwargs... how to support it cleanly?
There was a problem hiding this comment.
Simply:
elif kwargs.get('axis', 0) == 0 or args[0] == 0:
Because the other arguments are not integers. But yeah it's not foolproof in the general, but I cannot see any better way to handle that. Here it should be enough.
There was a problem hiding this comment.
Yes, not foolproff, but I agree there is no other simple way. And btw, elif kwargs.get('axis', 0) == 0 or args and args[0] == 0: as there may be no arguments.
Or even kwargs.get('axis', args[0] if args else 0) == 0 - I myself like it better.
There was a problem hiding this comment.
And after thinking a bit more, I would say we should either make separate functions for separate pandas ones, or disallow *args completely. For example, Series.apply(func, convert_dtype=True, args=(), **kwds) has the first argument as bool which can be passed value 0, and this can lead to very subtle bugs.
There was a problem hiding this comment.
I agree that we could disallow *args completely. We should just output a clear message that positional arguments are not supported with tqdm_pandas. This would be the best I think.
Current coverage is 88.77% (diff: 0.00%)@@ master #244 diff @@
==========================================
Files 7 7
Lines 485 606 +121
Methods 0 0
Messages 0 0
Branches 89 130 +41
==========================================
+ Hits 439 538 +99
- Misses 44 66 +22
Partials 2 2
|
|
And this certainly still doesn't work for |
|
Well, there are other subtleties: for example, one can pass named axis (e.g. |
|
Ah yes in this case it will be bothersome to handle all these special cases... |
|
@casperdcl What do you think of this PR? Can we merge it in? |
|
lemme send PR |
|
Thank's @CrazyPython :) |
|
Any updates on merging this?.. |
|
@aplavin merge typo PR |
Grammar fix
|
Casper seems to be busy and I'm moving to another country so I don't yet
|
|
|
in #299. |
Note, this is still not an always-correct solution! May be we need to have separate logic for apply/map/applymap?..