You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When using pd.DataFrame.describe, if your percentiles are different only at the 4th decimal place, a ValueError is thrown because the the percentiles that vary at the 4th decimal place become the same value.
In [1]: s = Series(np.random.randn(10))
In [2]: s.describe()
Out[2]:
count 10.000000
mean 0.291571
std 1.057143
min -1.453547
25% -0.614614
50% 0.637435
75% 0.968905
max 1.823964
dtype: float64
In [3]: s.describe(percentiles=[0.0001, 0.0005, 0.001, 0.999, 0.9995, 0.9999])
Out[3]:
count 10.000000
mean 0.291571
std 1.057143
min -1.453547
0.0% -1.453107
0.1% -1.451348
0.1% -1.449149
50% 0.637435
99.9% 1.817201
100.0% 1.820583
100.0% 1.823288
max 1.823964
dtype: float64
I have a bit of doubts if a unique index is an issue here. Firstly, a user can very well call s.describe(percentiles = [0.1, 0.1, 0.5]). Secondly, describe is not a function people usually use to calculate percentiles. So a pretty output might be more important than an exact percentile identifier. (But it's only a humble opinion.)
@ghost. Your function pretty_name has a minor issue:
I'm not sure if this isn't an overkill.
But if this is the expected behaviour and nobody works on this issue (ghost's pull request has been cancelled, I guess), I can submit it.
a small issue is that to actually get at these fields you have to use an exactly formatted label - if this is for display purposes only then that doesn't matter
Yes. This was one of the reasons I preferred to ask first.
But I don't think people really use it like this. (At least I can't think of a situation I'd need to use it.)
I think that assuming this is for display purposes only might get us into
trouble later on. I'm sure that if it's there, some people use it for more
than display purposes.
Yes. This was one of the reasons I preferred to ask first.
But I don't think people really use it like this. (At least I can't think
of a situation I'd need to use it.)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #13104 (comment)
I was about to submit a pull request but discovered that the rounding is only a part of the problem here. The other part got slightly overlooked. Namely, users themselves can supply non-unique percentiles. It works fine with Series
s = pd.Series(np.arange(11))
s.describe(percentiles = [0.1, 0.2, 0.2])
Out[52]:
count 11.000000
mean 5.000000
std 3.316625
min 0.000000
10% 1.000000
20% 2.000000
20% 2.000000
50% 5.000000
max 10.000000
but not with DataFrame (or other multidimensional objects):
df = pd.DataFrame(np.arange(11))
df.describe(percentiles = [0.1, 0.2, 0.2])
# ... longer traceback ...
ValueError: cannot reindex from a duplicate axis
.describe() internally uses pd.concat() for DataFrames and it breaks with non-unique entries in indexes (here, percentiles identifiers).
So, what would be the most sensible action when a user supplies non-unique percentiles:
raising a ValueError with an informative message (and if so, for all objects or excluding Series?)
There's a third option. Use the unique percentiles and issue a warning to
the user that they provided non-unique percentiles.
Personally, I'm in favor of raising a ValueError for all objects so that
things are standardized and to ensure people are getting the output they
expect.
I was about to submit a pull request but discovered that the rounding is
only a part of the problem here. The other part got slightly overlooked.
Namely, users themselves can supply non-unique percentiles. It works fine
with Series
s = pd.Series(np.arange(11))
s.describe(percentiles = [0.1, 0.2, 0.2])
Out[52]:
count 11.000000
mean 5.000000
std 3.316625
min 0.000000
10% 1.000000
20% 2.000000
20% 2.000000
50% 5.000000
max 10.000000
but not with DataFrame (or other multidimensional objects):
I'm actually not in favor of the warning because (1) it is only going to be
raised once; (2) we don't know how the user will use the percentiles (e.g.do
they iterate through them in the describe output?); and (3) it permits
sloppy coding that could cause someone problems later.
If you want the ability for duplicate percentiles, then how about a kwarg
like warn_on_dupe so that the default is an error and someone has to
explicitly request duplicates?
I was only proposing the warning for completeness. That being said, the
percentiles should be sorted so that the index is monotonic.
Slightly off-topic: if I keep finding other bugs and can fix them, should I open a new issue to discuss the code changes I propose? Or is it enough to discuss it in a pull request comments?
As an illustration, another bug with describe, this time unrelated to percentiles:
df=pd.DataFrame({'A': list("BCDE"), 0: [1,2,3,4]})
df.describe()
# long traceback listing several internal functionsValueError: Bufferdtypemismatch, expected'Python object'butgot'long'
I've found the source of the error and have a seemingly reasonable solution (it still needs testing, though).
Slightly off-topic: if I keep finding other bugs and can fix them, should
I open a new issue to discuss the code changes I propose? Or is it enough
to discuss it in a pull request comments?
As an illustration, another bug with describe, this time unrelated to
percentiles:
df = pd.DataFrame({'A': list("BCDE"), 0: [1,2,3,4]})
df.describe()# long traceback listing several internal functionsValueError: Buffer dtype mismatch, expected 'Python object' but got 'long'
I've found the source of the error and have a seemingly reasonable
solution (it still needs testing, though).
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #13104 (comment)
Yes it's distinct and I don't want to discuss it here. But I was going to fix it in the same commit because the code changes will probably be contained within the function describe.
My question was essentially: does any code change require opening a new issue? Wiki says not necessarily but mentions only insignificant changes.
Activity
blalterman commentedon May 11, 2016
Same thing with 0.0005 and 0.001.
On Tue, May 10, 2016 at 8:53 PM teaspreader notifications@github.com
wrote:
pijucha commentedon May 17, 2016
If I may add my 2 cents:
I have a bit of doubts if a unique index is an issue here. Firstly, a user can very well call
s.describe(percentiles = [0.1, 0.1, 0.5]). Secondly,describeis not a function people usually use to calculate percentiles. So a pretty output might be more important than an exact percentile identifier. (But it's only a humble opinion.)@ghost. Your function
pretty_namehas a minor issue:I wrote another function which finds a cutoff precision for float percentiles (and rounds them):
and also does some padding:
I'm not sure if this isn't an overkill.
But if this is the expected behaviour and nobody works on this issue (ghost's pull request has been cancelled, I guess), I can submit it.
jreback commentedon May 17, 2016
@pijucha you can submit if you'd like, still some comments outstanding on #13132
pijucha commentedon May 19, 2016
I'd better ask before I submit. Does anyone has an opinion on whether such a formatting (padding with blanks to align decimal points) is acceptable?
@jreback
jreback commentedon May 19, 2016
aligning on decimal point is fine
I think you should have all the same number of digits
to the right of decimal point as well
0.01,0.05,0.10,50.00,99.90,99.95,99.99
as they line up better
pijucha commentedon May 19, 2016
OK. Thanks for the comment.
blalterman commentedon May 19, 2016
Personally, I would prefer 0.9 over 0.90 because it implies less visual
clutter and I don't think we're concerned about sig figs.
On Wed, May 18, 2016 at 10:15 PM pijucha notifications@github.com wrote:
jreback commentedon May 19, 2016
maybe just leave the sig figs like u have them but align on decimals and also align the % - might be a nicer look
pijucha commentedon May 19, 2016
All right. I'm posting it once more to give some visual comparison.
jreback commentedon May 19, 2016
a small issue is that to actually get at these fields you have to use an exactly formatted label - if this is for display purposes only then that doesn't matter
s[' 0.1 %'] might be a bit awkward
pijucha commentedon May 19, 2016
Yes. This was one of the reasons I preferred to ask first.
But I don't think people really use it like this. (At least I can't think of a situation I'd need to use it.)
blalterman commentedon May 19, 2016
I think that assuming this is for display purposes only might get us into
trouble later on. I'm sure that if it's there, some people use it for more
than display purposes.
On Wed, May 18, 2016 at 10:51 PM pijucha notifications@github.com wrote:
pijucha commentedon May 19, 2016
True. I'm giving up on padding. Without blanks, it's both simpler to use and easier to code.
Now, I'm a bit in favour of Version 2. (Version 1 automatically adds a decimal point to all entries if there is at least one non-integer.)
jreback commentedon May 19, 2016
I think I like version 2
jreback commentedon May 19, 2016
@jorisvandenbossche @TomAugspurger @sinhrks
sinhrks commentedon May 19, 2016
+1 for version2, as readabily looks important rather than precision consistency.
pijucha commentedon May 24, 2016
I was about to submit a pull request but discovered that the rounding is only a part of the problem here. The other part got slightly overlooked. Namely, users themselves can supply non-unique percentiles. It works fine with Series
but not with DataFrame (or other multidimensional objects):
.describe()internally usespd.concat()for DataFrames and it breaks with non-unique entries in indexes (here, percentiles identifiers).So, what would be the most sensible action when a user supplies non-unique percentiles:
blalterman commentedon May 24, 2016
There's a third option. Use the unique percentiles and issue a warning to
the user that they provided non-unique percentiles.
Personally, I'm in favor of raising a ValueError for all objects so that
things are standardized and to ensure people are getting the output they
expect.
On Tue, May 24, 2016 at 1:22 AM pijucha notifications@github.com wrote:
jreback commentedon May 24, 2016
I think raising on duplicated percentiles if fine.
pijucha commentedon May 24, 2016
I like @bla1089 's idea (unique percentiles plus a warning). So if it's also acceptable, I'd go for it.
And another minor issue: I'd always sort percentiles on output. Now, it depends on whether you enter 0.5, so some weird reordering may happen:
jreback commentedon May 24, 2016
sorting is fine. I am not really in favor of duplicates. yes they are handled generally, but not particularly useful IMHO.
blalterman commentedon May 24, 2016
I'm actually not in favor of the warning because (1) it is only going to be
raised once; (2) we don't know how the user will use the percentiles (e.g.do
they iterate through them in the describe output?); and (3) it permits
sloppy coding that could cause someone problems later.
If you want the ability for duplicate percentiles, then how about a kwarg
like
warn_on_dupeso that the default is an error and someone has toexplicitly request duplicates?
I was only proposing the warning for completeness. That being said, the
percentiles should be sorted so that the index is monotonic.
On Tue, May 24, 2016, 11:11 Jeff Reback notifications@github.com wrote:
pijucha commentedon May 25, 2016
Yes, this sounds reasonable. Exceptions then.
Slightly off-topic: if I keep finding other bugs and can fix them, should I open a new issue to discuss the code changes I propose? Or is it enough to discuss it in a pull request comments?
As an illustration, another bug with
describe, this time unrelated to percentiles:I've found the source of the error and have a seemingly reasonable solution (it still needs testing, though).
blalterman commentedon May 25, 2016
This seems reasonably distinct and should probably be handled on its own.
On Wed, May 25, 2016, 10:06 pijucha notifications@github.com wrote:
pijucha commentedon May 25, 2016
Yes it's distinct and I don't want to discuss it here. But I was going to fix it in the same commit because the code changes will probably be contained within the function
describe.My question was essentially: does any code change require opening a new issue? Wiki says not necessarily but mentions only insignificant changes.
jreback commentedon May 25, 2016
generally we like to have issues to go with PR's.
pijucha commentedon May 25, 2016
Ok, thanks. I'll open a new one then.
BUG: Fix describe(): percentiles (#13104), col index (#13288)