/matplotlib

Clarify Axes.hexbin *extent* docstring #6842

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
  v2.0.0
5 participants
@LindyBalboa @tacaswell @Kojoley @mdboom @QuLogic
@LindyBalboa

LindyBalboa commented Jul 26, 2016

Without looking at the source, it was unclear that extent expects the
limits to be given as powers of ten if the axis scale is set to 'log'.
E.g., using 3 to represent 1000.

Also went through the file and standardized quote usage.

  • '': for use around parameters
  • "": for use around sentences

Resolves: #6479

mdboom added the needs_review label Jul 26, 2016

@Kojoley Kojoley commented on an outdated diff Jul 26, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
@@ -4820,11 +4827,11 @@ def get_interp_point(ind):
self.autoscale_view()
return collection
- @unpack_labeled_data(replace_names=["y", "x1", "x2", "where"],
- label_namer=None)
+ @unpack_labeled_data(replace_names=['y', 'x1', 'x2', 'where'],
+ label_namer=none)
@Kojoley

Kojoley Jul 26, 2016

Member

Is this working?

@Kojoley Kojoley commented on an outdated diff Jul 26, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
@docstring.dedent_interpd
- def fill_betweenx(self, y, x1, x2=0, where=None,
- step=None, **kwargs):
+ def fill_betweenx(self, y, x1, x2=0, where=none,
+ step=none, **kwargs):
@Kojoley

Kojoley Jul 26, 2016

Member

Same as above

@Kojoley Kojoley commented on an outdated diff Jul 26, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
@@ -530,9 +530,9 @@ def legend(self, *args, **kwargs):
labels = [handle.get_label() for handle in handles]
for label, handle in zip(labels[:], handles[:]):
if label.startswith('_'):
- warnings.warn('The handle {!r} has a label of {!r} which '
- 'cannot be automatically added to the '
- 'legend.'.format(handle, label))
+ warnings.warn("The handle {!r} has a label of {!r} which "
+ "cannot be automatically added to the "
+ "legend.'.format(handle, label))
@Kojoley

Kojoley Jul 26, 2016 • edited

Member

Here is something strange. You have inverted all but not the last apostrophe.
@LindyBalboa

LindyBalboa commented Jul 26, 2016

I noticed the None/none variance in several places, but didn't think I touched any of them. And you are right, somehow I missed that last one. I will look into the None/none issue and fix that quote tomorrow.

I knew my first PR was going to have some silly mistakes!
@tacaswell
Owner

tacaswell commented Jul 28, 2016

Looks like there are a bunch of pep8 whitespace related issues.

======================================================================
FAIL: matplotlib.tests.test_coding_standards.test_pep8_conformance_installed_files
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 244, in test_pep8_conformance_installed_files
    expected_bad_files=expected_bad_files)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 143, in assert_pep8_conformance
    assert_equal(result.total_errors, 0, msg)
AssertionError: 7 != 0 : Found code syntax errors (and warnings):
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:87:72: E502 the backslash is redundant between brackets
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:88:75: E502 the backslash is redundant between brackets
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:359:76: W291 trailing whitespace
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:2815:68: W291 trailing whitespace
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:2816:69: W291 trailing whitespace
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:4133:65: W291 trailing whitespace
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:4134:63: W291 trailing whitespace

the enforcement of pep8/pycodestyle is annoying, but long term it is very helpful in a codebase with many people touching the code.

@tacaswell tacaswell commented on an outdated diff Jul 28, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
else:
- raise ValueError("Using arbitrary long args with data is not "
- "supported due to ambiguity of arguments.\nUse "
+ raise ValueError("Using arbitrary long args with data is not " \
@tacaswell

tacaswell Jul 28, 2016

Owner

remove the \

@tacaswell tacaswell commented on an outdated diff Jul 28, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
@@ -530,9 +530,9 @@ def legend(self, *args, **kwargs):
labels = [handle.get_label() for handle in handles]
for label, handle in zip(labels[:], handles[:]):
if label.startswith('_'):
- warnings.warn('The handle {!r} has a label of {!r} which '
- 'cannot be automatically added to the '
- 'legend.'.format(handle, label))
+ warnings.warn("The handle {!r} has a label of {!r} which "
@tacaswell

tacaswell Jul 28, 2016

Owner

This switches the quotes the other way than the rest of the changes.

@tacaswell tacaswell and 1 other commented on an outdated diff Jul 28, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
@@ -2394,7 +2394,8 @@ def stem(self, *args, **kwargs):
linestyle, linemarker, linecolor = \
_process_plot_format(linefmt)
else:
- linestyle, linemarker, linecolor = _process_plot_format(linefmt)
@tacaswell

tacaswell Jul 28, 2016

Owner

we are trying to remove \ from the code base, please do not add them.

@LindyBalboa

LindyBalboa Jul 28, 2016 • edited

I actually returned it to the way that it originally was (after I changed it). I guess the issue around these lines with the width.

And what about multi-line strings? Should they be wrapped in ( ) instead of backslashes?

@tacaswell

tacaswell Jul 28, 2016

Owner

Yes, () is the preferred method of line continuation.

@tacaswell tacaswell commented on an outdated diff Jul 28, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
*extent*: [ *None* | scalars (left, right, bottom, top) ]
The limits of the bins. The default assigns the limits
- based on gridsize, x, y, xscale and yscale.
+ based on *gridsize*, *x*, *y*, *xscale* and *yscale*.
+
+ If *xscale* or *yscale* is set to 'log', the limit is
@tacaswell

tacaswell Jul 28, 2016

Owner

This seems to have broken sphinx:

Warning, treated as error:
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:docstring of matplotlib.axes.Axes.hexbin:105: WARNING: Block quote ends without a blank line; unexpected unindent.

@tacaswell tacaswell modified the milestone: 2.1 (next point release), 2.0.1 (next bug fix release) Jul 28, 2016

@tacaswell tacaswell commented on an outdated diff Jul 28, 2016

View full outdated diff
lib/matplotlib/axes/_axes.py
*extent*: [ *None* | scalars (left, right, bottom, top) ]
The limits of the bins. The default assigns the limits
- based on gridsize, x, y, xscale and yscale.
+ based on *gridsize*, *x*, *y*, *xscale* and *yscale*.
+
+ If *xscale* or *yscale* is set to 'log', the limit is
+ expected as a power of 10. E.g. for x-limits of 1 and
@tacaswell

tacaswell Jul 28, 2016

Owner

🚲 🏠 Should this be '..the limits are the power to raise 10 to to compute the range of the bins.'? I would read 'expected as a power of 10' to mean the inputs must be of the form [10, 100, 1000, ...].
@tacaswell
Owner

tacaswell commented Jul 28, 2016

The documentation changes seem very good, however it is greatly obscured by the changes to the quotes.

Would it be possible to separate those two sets of changes?
LindyBalboa Clarify Axes.hexbin doctring *extent* option 
Without looking at the source, it was unclear that *extent* expects
the limites to be given as exponents of powers of 10 if the axis
scale is set to 'log'. E.g., using 3 to represent 1000.

Removed superfluous style change commits. Will open separate pull
request for that.

Resolves: #6479

All checks have passed

3 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.002%) to 70.447%
Details
bed7c7b
@LindyBalboa

LindyBalboa commented Jul 28, 2016

This was definitely a learning experience. I had to learn a lot about git, virtual environments, building/installing, testing. Yowza. And I definitely learned the importance of splitting up PRs. Thanks for the patience guys.
@tacaswell
Owner

tacaswell commented Jul 28, 2016

@LindyBalboa Everyone starts someplace! Sorry the learning curve was steep.

@tacaswell tacaswell merged commit af87fb4 into matplotlib:master Aug 2, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 70.447%
Details

tacaswell removed the needs_review label Aug 2, 2016

@tacaswell tacaswell added a commit that referenced this pull request Aug 2, 2016

@tacaswell tacaswell Merge pull request #6842 from LindyBalboa/fix_issue_6479 
DOC: Clarify Axes.hexbin *extent* docstring
 2811640
@tacaswell
Owner

tacaswell commented Aug 2, 2016

backported to v2.x as 2811640
@tacaswell
Owner

tacaswell commented Aug 2, 2016

@LindyBalboa Thank you (and thank you for going through the learning curve!)

Documentation is drastically undervalued and we very much appreciate your work on this.

I think this is your first contribution to mpl. Congratulation 🎉 and hopefully we will hear from you again soon!

@QuLogic QuLogic modified the milestone: 2.0 (style change major release), 2.0.1 (next bug fix release) Aug 2, 2016

LindyBalboa deleted the LindyBalboa:fix_issue_6479 branch Aug 2, 2016

QuLogic added the Documentation label Sep 6, 2016

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