Clarify the uses of whiskers float parameter. #7753

Merged
merged 1 commit into from Jan 12, 2017

Conversation

Projects
None yet
6 participants
Contributor

Carreau commented Jan 6, 2017

It is not clear (at least to a couple of non-native English speakers)
that the whiskers extend to the last data point. More precisely:

"If it's 1.5 times Q3-Q1 it should be symmetric"

The code does seem to limit the whisker to the last data point, and that
correspond to one of the usage of whiskers described by Wikipedia.

So fix docstrings.


Improvement thanks to @emilienschultz, but I already had a matplotlib clone and a dev install, so it was faster for me to fix it.

Current coverage is 62.12% (diff: 100%)

Merging #7753 into master will not change coverage

@@             master      #7753   diff @@
==========================================
  Files           174        174          
  Lines         56028      56028          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          34805      34805          
  Misses        21223      21223          
  Partials          0          0          

Powered by Codecov. Last update 1fa4dd7...a7b04b1

tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 6, 2017

tacaswell requested a review from phobson Jan 6, 2017

lib/matplotlib/axes/_axes.py
+ As a float, determines the reach of the whiskers to the first
+ (resp. last) datum past the first (resp. third) quartiles
+ (e.g., Q3 + whis*IQR, IQR = interquartile range, Q3-Q1,
+ whisker will be at the last data point of Q3 + whis*IQR).
@tacaswell

tacaswell Jan 6, 2017

Owner

"will be at last data point less than Q3 + whis*IQR."?

@Carreau

Carreau Jan 6, 2017

Contributor

Changed to your wording.

@Carreau

Carreau Jan 6, 2017

Contributor

(in both docstrings)

@phobson

phobson requested changes Jan 6, 2017 edited

I think this is headed in the right direction. I would recommend:

As a float, determines the reach of the whiskers to the beyond the first and third quartiles. In other words, where IQR is the interquartile range (Q3-Q1), the upper whisker will extend to last datum less than Q3 + whis*IQR). Similarly, the lower whisker will extend to the first datum greater than Q1 - whis*IQR.

Member

phobson commented Jan 6, 2017

FWIW, wikipedia currently explains this as:

the lowest datum still within 1.5 IQR of the lower quartile, and the highest datum still within 1.5 IQR of the upper quartile.

I think that would work as well (with attribution)

@Carreau Carreau Clarify the uses of whiskers float parameter.
It is not clear (at least to a couple of non-native English speakers)
that the whiskers extend to the last data point. More precisely:

> "If it's 1.5 times Q3-Q1 it should be symmetric"

The code does seem to limit the whisker to the last data point, and that
correspond to one of the usage of whiskers described by Wikipedia.

So fix docstrings.
97609a8
Contributor

Carreau commented Jan 12, 2017

@phobson pushed with your first phrasing. I find it clearer than the wikipedia one.

@phobson

Thanks!

Contributor

NelleV commented Jan 12, 2017

The failure is unrelated to the patch, so I'm merging.
Thanks @Carreau !

@NelleV NelleV merged commit 4d390fa into matplotlib:master Jan 12, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
coverage/coveralls Coverage increased (+0.004%) to 62.1%
Details

Carreau deleted the Carreau:improve-whiskers-docstrings branch Jan 12, 2017

Member

QuLogic commented Jan 30, 2017 edited

Backported to v2.0.x as f12fa6e.

@QuLogic QuLogic added a commit that referenced this pull request Jan 30, 2017

@NelleV @QuLogic NelleV + QuLogic Merge pull request #7753 from Carreau/improve-whiskers-docstrings
Clarify the uses of whiskers float parameter.
f12fa6e
Contributor

Carreau commented Jan 30, 2017

Backported to v2.0.x as f12fa6e.

Thanks.

Just a question: Isn't that a bit overkill for a patch release ? For a 2.x I would have understood, even if I believe this would not fit a backport to a minor in IPython. Or I'm misinterpreting the numbering of matplotlib.

Member

QuLogic commented Jan 31, 2017

I'm just going by the milestone, but documentation corrections are generally safe to backport.

Contributor

Carreau commented Jan 31, 2017

I'm just going by the milestone,

Yes, I understand that, and thanks for doing it.

but documentation corrections are generally safe to backport.

That's not a question of "safety", it's also a question of maintainer time and message to the community.
If backport everything and spend 1h backporting 10 PRs then that's 1h less of actual code review.
Plus it's not helping to convey a clear message to the community as to what will be better in 2.1.

Typically #7766 was deprecation warnings, was it really worth backporting ? Or is #7913 (quiver docs and example), which for me just an addition (and a carrot to upgrade on 2.1) if it had been only on 2.1

I just don't want to guys to burnout by doing too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment