Inset axes bug and docs fix #14278

clbarnes · 2019-05-20T18:04:19Z

Previously, Axes.indicate_inset would fail if the optional inset_ax argument was not given; now it doesn't. Test included.
Previously, Axes.indicate_inset* methods' docstring incorrectly stated that they returned a tuple; now they correctly state that they return a list (which is empty if inset_ax is None).

Ideally, I would have preferred them to return a 4-tuple as documented (or None, in the case of indicate_inset with no inset_ax). However, that would technically be a breaking change: bringing the docs in line with the implementation seemed a less contentious idea that bringing the code in line with the docs.

That said, these methods are documented as experimental, so if others agree with my API preference, it may be worth raising a new issue to use tuples instead.

jklymak · 2019-05-20T20:57:24Z

This is new and experimental so I think breaking changes are still fair game. Thanks for looking at this!

clbarnes · 2019-05-20T23:39:32Z

Should it go in as is (with the docs and implementation both using a list, either 4-length or empty), or should I revert the docs to the optional 4-tuple and then match the implementation to that? I think the optional 4-tuple is more true to what it's representing (index has semantic meaning, and it's either 4-length or nothing), although it's arguable that the list is more pythonic (empty lists being falsey anyway, and still behaving in an unsurprising way for iteration, concatenation etc).

clbarnes · 2019-05-21T00:02:25Z

Lastly, as this is new-ish API surface for versions of matplotlib which will only be targeting python >=3.6, is there any appetite for PEP-484 type annotations?

clbarnes · 2019-05-21T19:16:41Z

New commits implement the documented tuple rather than documenting the implemented list. Also adds type annotations.

jklymak · 2019-05-26T15:32:39Z

lib/matplotlib/tests/test_axes.py

@@ -5935,6 +5935,33 @@ def test_tick_padding_tightbbox():
    assert bb.y0 < bb2.y0


+def test_inset():
+    """Regression test: failure described in for issue #14275"""


Please don't reference issues in the comments.. Every commit adds a notification message in the issue ;-)

Yes, I noticed that! Sorry. I think it's valuable to reference the issue in regression tests, but next time I won't include the #, to stop github picking it up.

#12475 mentions a doc problem, so the test can't really be testing for that. I'd rather just explain what you're testing in the docstring.
Also "Regression test" basically adds no information: most tests are regression tests...

jklymak · 2019-05-26T15:33:56Z

lib/matplotlib/axes/_axes.py

+            edgecolor='0.5',
+            alpha: float = 0.5,
+            zorder: float = 4.99, **kwargs
+        ) -> Tuple[


I've not ever seen this syntax before. Are we supporting it in matplotlib? @anntzer @timhoffm ?

These are PEP-484 type hints; they're immensely helpful to downstream users (in my opinion). Matplotlib which has a deep, nested class structure, and the docs can be tricky to navigate: knowing exactly what you're getting out of a function call, and your IDE being able to tell you if you're using it wrong, increases discoverability of the library and productivity of the user. A systematic, well-specified, core python way of documenting types in signatures are also just better than the somewhat vague numpydoc conventions for docstrings. Sphinx can pick it up with an extension, the whole module path is necessarily resolved, and the IDE support is much broader. We've been messing around with rst, numpydoc, googledoc for years: now we have one obvious way to do it.

It might be nice if eventually most of matplotlib's API surface was covered, but there's no need to do it all in one go - because it's not checked at runtime and not enforced they can be phased in gradually. As this is new API surface which will only ever be used in python 3.6+, it seemed a good corner of the code to start!

Given that @jklymak asked me for my opinion... I personally think the design of PEP484 (and followup PEPs, especially 560, 561, 563) type hints is poor and am not particularly looking forward to adding type hints everywhere (no, I don't have any concrete alternative proposal).
On the other hand, I guess type hints are going one day or another to be everywhere :/, so I'm not going to block their addition either. (I guess some here may be a bit surprised to hear that there actually are some new and shiny Python features that I don't like :p)

(FWIW I would have been more interested by work on type inference (à la C++ "almost always auto" style).)

I think we need an internal decision if and how we want to add type hints. See also @tacaswell's comment: #13798 (comment).

jklymak · 2019-05-26T15:37:47Z

doc/api/next_api_changes/2019-05-21-CB.rst

+`matplotlib.axes.Axes.indicate_inset_zoom` were documented as returning
+a 4-tuple of `matplotlib.patches.ConnectionPatch`es, where in fact they
+returned a 4-length list.
+


I guess I'm not seeing the difference here. What is the advantage of the change below? If its just because we don't think a list is a tuple, then I'd argue just changing the docs to say it is a list. But practically, what is the difference?

I'm not sure I understand. Lists and tuples are different. They do different things. They have different methods. At the very least, the documentation should match the implementation.

If you tell someone you're giving them a duck and then show up with a cat, they're going to be annoyed that they've spent some time digging a pond.

I'd argue just changing the docs to say it is a list

This is what I did initially. However, in this case, the return value has a fixed length and the indices are meaningful, so an optional tuple is closer to the meaning of what's being represented. I suggested the change in an earlier comment and said my only reservation was about it being a breaking API change, and you responded that breaking changes were fair game as it's an experimental feature.

Ok I was looking at the wrong change. In general it’s better not to have too many style changes unless they help pass PEP8 with fewer exceptions.

I’m not clear if a tuple or list is particularly better here. I guess the fact that it’s always a four tuple would argue for that. My apologies that my previous comment was taken to mean I thought it should be changed, just that it could if there is a good reason.

Returning a tuple makes sense semantically.

anntzer · 2019-05-26T18:20:51Z

lib/matplotlib/axes/_axes.py

@@ -31,9 +28,12 @@
 import matplotlib.ticker as mticker
 import matplotlib.transforms as mtransforms
 import matplotlib.tri as mtri
-from matplotlib.container import BarContainer, ErrorbarContainer, StemContainer
+import numpy as np


please leave the numpy imports at the top (between the stdlib imports and the matplotlib imports).

anntzer · 2019-05-26T18:23:11Z

lib/matplotlib/axes/_axes.py

+            Optional[
+                Tuple[
+                    mpatches.ConnectionPatch, mpatches.ConnectionPatch,
+                    mpatches.ConnectionPatch, mpatches.ConnectionPatch


If you're going to go to the bother of having closing brackets each on their own line (not really sure it's worth it in this case), I would suggest adding trailing commas as well (minimizing diffs, yada yada).

anntzer · 2019-05-26T18:26:56Z

lib/matplotlib/tests/test_axes.py

+    fig.canvas.draw()
+    xx = np.array([[1.5, 2.],
+                   [2.15, 2.5]])
+    assert (np.all(rec.get_bbox().get_points() == xx))


extra parentheses

clbarnes · 2019-06-11T18:46:10Z

Rebased on master and addressed @anntzer 's style comments. I'll squash down the commits before it goes in.

Happy to polish this off in whichever way you want, just give me a directive for

Whether you will allow type annotations
Whether you prefer A) the documentation changing to meet the implementation (i.e. a 4-length or empty list of lines), or B) the implementation changing to meet the documentation (i.e, a 4-tuple of lines or None), where B is probably the more semantically correct outcome.

Also, the docs build is failing and I don't know why - any pointers? Locally, I get

generating gallery for gallery/text_labels_and_annotations... [ 45%] font_file.py                                                             
Exception occurred:
  File "/home/barnesc/.pyenv/versions/mpl/lib/python3.7/site-packages/matplotlib/font_manager.py", line 1328, in get_font
    return _get_font(filename, hinting_factor)
TypeError: First argument must be a path or file object reading bytes
The full traceback has been saved in /tmp/sphinx-err-cq03b1_y.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Makefile:30: recipe for target 'html' failed
make: *** [html] Error 2

anntzer · 2019-06-11T19:25:21Z

I added whether to add type hints on the agenda of the weekly dev call (next week).

clbarnes · 2019-06-11T19:35:31Z

Thanks! To be clear, I don't think that it should be a priority to sweep through the entire codebase adding them - nor does that need to be the case for them to be added in this PR. They can be added incrementally wherever it's helpful, if we happen to be in that area of the code doing other things. It's mainly a question of whether the core maintainers prefer to affirmatively avoid them.

For what it's worth, we are gradually adding them to our own project and have found a lot of dead code, bugs, inaccurate documentation etc. because of it; it's been very valuable from a maintainability perspective.

efiring · 2019-06-17T19:43:52Z

I think that introducing type annotations now would be a mistake. They add yet another level of complexity and bulk to a codebase that is already hard to master, especially considering that it remains a volunteer project.

clbarnes · 2019-06-17T21:05:48Z

IMHO, they could make the codebase considerably easier to master. Right now, IDEs struggle to autocomplete ambiguous return types (and/or they incorrectly infer them, leading to the dreaded yellow squigglies), and a lot of effort is put into explaining types in docstrings which are finnicky to make machine-readable. #14275, which precipitated this PR, is a mismatch in docs and implementation which would have been caught in a snap given type annotations.

There are definitely places where the return type would have to be very complex because it returns different things in different situations - most of these won't change due for compatibility reasons, but encouraging type annotations in new code would dissuade people from making similar ambiguous API designs in the future, which is a positive. Annotations shouldn't have to change with any regularity, and there's no hurry or even necessity to tackle the older code.

clbarnes · 2019-06-24T14:14:11Z

I think the controversial nature of the type hints are holding this up, so I'll strip out that commit. However, I think it should be discussed further and so would appreciate it if anyone with opinions on the matter would check in at #13798 !

Unless anyone gets back to me with a decision to the contrary, I'll keep the fix here as an optional 4-tuple (as originally documented) rather than a zero-or-4-length-list (as originally implemented).

Also rebased on master and squashed review-fix commits.

timhoffm

Still approving after all changes. 😄

jklymak · 2019-07-05T15:15:34Z

Seems fine to me, but must pass CI?

See matplotlib#14275 inset_ax argument is meant to be optional, but currently fails if not given.

See matplotlib#14275 1. Methods now return a tuple as documented, instead of a list. 2. indicate_inset() now does not error when optional inset_ax is not passed, but returns None instead of a tuple.

clbarnes · 2019-07-06T04:46:07Z

must pass CI

Done, I'd written some markdown in a ReST file and it broke the docs build.

jklymak · 2019-07-06T17:48:46Z

Thanks a lot @clbarnes !

timhoffm approved these changes May 20, 2019

View reviewed changes

clbarnes force-pushed the 14275-inset-axes branch from 5bac860 to 23f56c8 Compare May 21, 2019 19:14

clbarnes force-pushed the 14275-inset-axes branch from 23f56c8 to 63904bf Compare May 21, 2019 19:37

jklymak reviewed May 26, 2019

View reviewed changes

anntzer reviewed May 26, 2019

View reviewed changes

clbarnes force-pushed the 14275-inset-axes branch from 63904bf to cc1f9f8 Compare June 11, 2019 18:40

clbarnes mentioned this pull request Jun 24, 2019

Add PEP484 type hints to the code (For IDE autocompletion / hints) #13798

Closed

clbarnes force-pushed the 14275-inset-axes branch 2 times, most recently from 81399c6 to 11b3f94 Compare June 24, 2019 15:19

tacaswell added this to the v3.2.0 milestone Jun 27, 2019

timhoffm approved these changes Jul 4, 2019

View reviewed changes

TST: Axes.indicate_inset failure

4e46968

See matplotlib#14275 inset_ax argument is meant to be optional, but currently fails if not given.

clbarnes force-pushed the 14275-inset-axes branch from 11b3f94 to c5c1b2a Compare July 6, 2019 03:51

FIX: Axes.indicate_inset* methods

86f5fb0

See matplotlib#14275 1. Methods now return a tuple as documented, instead of a list. 2. indicate_inset() now does not error when optional inset_ax is not passed, but returns None instead of a tuple.

clbarnes force-pushed the 14275-inset-axes branch from c5c1b2a to 86f5fb0 Compare July 6, 2019 04:12

jklymak approved these changes Jul 6, 2019

View reviewed changes

jklymak merged commit dfb3bf0 into matplotlib:master Jul 6, 2019

timhoffm mentioned this pull request Jul 16, 2020

Docs: Axes.inset_rectangle is referred to but does not exist #14275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inset axes bug and docs fix #14278

Inset axes bug and docs fix #14278

clbarnes commented May 20, 2019

jklymak commented May 20, 2019

clbarnes commented May 20, 2019

clbarnes commented May 21, 2019

clbarnes commented May 21, 2019

jklymak May 26, 2019

clbarnes May 26, 2019

anntzer May 26, 2019

jklymak May 26, 2019

clbarnes May 26, 2019

anntzer May 26, 2019

timhoffm May 26, 2019

jklymak May 26, 2019

clbarnes May 26, 2019

jklymak May 26, 2019

anntzer May 26, 2019

anntzer May 26, 2019

anntzer May 26, 2019

anntzer May 26, 2019

clbarnes commented Jun 11, 2019 •

edited

Loading

anntzer commented Jun 11, 2019

clbarnes commented Jun 11, 2019

efiring commented Jun 17, 2019

clbarnes commented Jun 17, 2019

clbarnes commented Jun 24, 2019 •

edited

Loading

timhoffm left a comment

jklymak commented Jul 5, 2019

clbarnes commented Jul 6, 2019

jklymak commented Jul 6, 2019

Inset axes bug and docs fix #14278

Inset axes bug and docs fix #14278

Conversation

clbarnes commented May 20, 2019

jklymak commented May 20, 2019

clbarnes commented May 20, 2019

clbarnes commented May 21, 2019

clbarnes commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clbarnes commented Jun 11, 2019 • edited Loading

anntzer commented Jun 11, 2019

clbarnes commented Jun 11, 2019

efiring commented Jun 17, 2019

clbarnes commented Jun 17, 2019

clbarnes commented Jun 24, 2019 • edited Loading

timhoffm left a comment

Choose a reason for hiding this comment

jklymak commented Jul 5, 2019

clbarnes commented Jul 6, 2019

jklymak commented Jul 6, 2019

clbarnes commented Jun 11, 2019 •

edited

Loading

clbarnes commented Jun 24, 2019 •

edited

Loading