DOCS: add examples of how one "should" use Bbox #17090

brunobeltran · 2020-04-10T04:50:34Z

PR Summary

A tutorial on how to use something as simple as a Bbox, seems unecessary right? But subtle, innocuous word choices in the docs of Bbox made it really hard for me to discover the "intended" workflow.

For example, it took a while to notice that Bbox.null() exists when just scrolling through, even though by any measure this is probably the most "important" Bbox. Noticing that update_from_path was looking at just the vertices of the path, and not the "true" path extents, required me to dive into _path.h, which also felt like a failure of documentation (fixed in #16832, which hopefully gets merged any day now). I could go on, but the point is that a short "Examples" section in the docstring would have solved all these issues.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

lib/matplotlib/transforms.py

timhoffm · 2020-04-10T08:03:34Z

lib/matplotlib/transforms.py

+
+        >>> Bbox.intersection(Bbox.null(), Bbox.null())
+        Bbox([[-inf, -inf], [inf, inf]])
+


Is this a reasonable behavior? Seems to contradict the semantics of update_from_data_xy.

Yeah part of why I'm documenting this is because this behavior was very unexpected. I expected Bbox.null() to be the "zero" object of the intersection/union ring over the Bbox's.

But it's not technically a bug per se since it's not documented, so I didn't open an issue.

That said, I'd be happy to make a separate PR to fix this "bug", if other people also agree that it shouldn't be this way, but don't want to get bogged down talking about deprecation timelines, etc. here, would rather open a separate issue.

This and below clearly look like bugs, but I'm fine documenting these here and changing them later.

jklymak · 2020-04-10T14:56:02Z

This looks like a valuable addition, but not sure about the format. Should this be an example, and the bbox docs refer to the example? We dont' have a lot of docs that scroll through the whole functionality as a series of examples in the actual docstring.

brunobeltran · 2020-04-10T17:40:29Z

@jklymak I would guess this is less intrusive, since this are largely "internal-facing" docs, and so a new developer is more likely to help(Bbox) than they are to look through documentation examples. (Whereas for user-facing docs, I agree that tutorials/examples are the best way to give visibility to features!)

tacaswell · 2020-04-10T21:34:45Z

I think this renders nicely: https://34270-1385122-gh.circle-artifacts.com/0/doc/build/html/api/transformations.html?highlight=bbox#matplotlib.transforms.Bbox

Given the internal facing nature of this, I think it also is nice to have the algebra directly in the source (not linked).

I'm in favor of documenting the behavior and then sorting out if we can fix it (and if it changed recently) in a follow up PR.

tacaswell

Technically we only need 1 approval for doc changes, but I'm going to hold off merging as Tim and Jody have some concerns.

timhoffm

I'm ok with adding these examples here. It's a bit lengthy but still ok, You might want to add sections for structuring (technically there are no subsections in numpydoc, but we can use bold paragraphs as subtitles:

Examples
--------
**Creating Bboxes**

...

**Modifying Bboxes**

 ...

Feel free to choose other subsection titles.

While this could be placed externally and linked, I don't think we have a dedicated suitable part of the docs to put it. There are the Examples which illustrate exemplary use cases, and there are the Tutorials which give a narrative introduction into a topic. The third and IMHO missing category would be comprehensive documentation of concepts in matplotlib, which are descriptive rather than narrative. This kind of docs is currently spread across various locations, some in Examples, some in tutorials, some in module docstrings and some in dedicated .rst files. I'd like to clean that up at some point, but up until then it's fine to have this in the docstring.

lib/matplotlib/transforms.py

brunobeltran · 2020-04-11T04:49:24Z

The third and IMHO missing category would be comprehensive documentation of concepts in matplotlib, which are descriptive rather than narrative.

I'm glad to see that I'm not the only one that feels this is the main missing component.

My thought was that by adding more fleshed out documentation to a few key classes little by little, we might eventually reach a point where this kind of "concept" documentation can be made surprisingly easily by stitching together the docstrings to these conceptually crucial classes in a way that's descriptive instead of tutorial/narrative driven.

Anywho, incorporated all your suggestions @timhoffm, thanks for the improvements!

anntzer · 2020-04-11T08:41:14Z

Very tangential note re: subsections in long docstrings: see numpy/numpydoc#216 if you have time to waste in another rabbit hole -- or perhaps try .. rubric:: / .. topic:: as suggested in that thread.

timhoffm · 2020-04-11T09:30:26Z

It could be interesting to investigate other subsection techniques. But that would be something for a separate PR; we've already used the bold paragraph way in a number of docstrings. And IMHO it's more readable in the the source than ReST directives.

lib/matplotlib/transforms.py

brunobeltran · 2020-04-11T20:04:04Z

Removed the unnecessarily inflammatory language, thanks @timhoffm.

I assume codecov is still broken? Since this is a docs PR lol

timhoffm

Sorry to be nitpicky, but I like to have the docs as clear as possible so that it‘s easy to understand for the user.

timhoffm · 2020-04-12T01:16:11Z

lib/matplotlib/transforms.py

+        >>> box
+        Bbox([[1.0, 1.0], [1.0, 1.0]])
+
+    .. warning:: Not setting ``ignore`` can have unexpected results.


Can you be more specific about this? Are you referring to the somewhat questionable behavior that the last state of ignore is used by default? If so, use something like:

It is recommended to always specify ignore explicitly. If not set, the value of ignore from the previous call will be reused.

I'm even more worried about users trying to use the fact that it's supposed to be the previous call's ignore value that is reused.

In practice, .ignore could have been called, or external code that touched the bbox could have changed the default value.

That happened to me when coding #16832, and it was a pain to track down.

I updated the warning to be very specific.

timhoffm · 2020-04-12T01:20:59Z

lib/matplotlib/transforms.py

+
+    **Create from collections of points**
+
+    The "empty" object for accumulating Bboxs is the null bbox, which has


This is more „properties of the null box“ and might be better moved there. While you are using the null box in the following example, the null boxes dimensions are not relevant there. Without knowing what a null box is, you can still „create a null box and update it with the points“.

It could be moved there, but I think it makes sense for the user to see "why" the null Bbox is the "empty" object. I mean they can always read past this information if it's not relevant for them?

My update is a compromise that doesn't emphasize the properties of the null Bbox until later, but still prints it, so that the user can infer why it is "like" the empty set if they "feel" like thinking that hard.

If you feel strongly that the new version still feels out-of-order, I'll change it :)

brunobeltran · 2020-04-12T02:06:28Z

Appreciate the nitpickyness @timhoffm, I am happy for this to take as long as it needs for us to get the best docs out!

Let me know if you have any further qualms about the new version.

story645 · 2020-04-12T04:55:08Z

Tangential tangential, but we've been putting some conceptual stuff in the blog at the moment (https://matplotlib.org/matplotblog/) but also like half of the entries in the tutorial section are really conceptual overviews 🤷‍♀️

lib/matplotlib/transforms.py

brunobeltran · 2020-04-13T02:58:23Z

Git's auto-accept-code-review button dumped whitespace all over the place, had to rebase, sorry.

brunobeltran force-pushed the bbox-docs branch 2 times, most recently from b8cd9e4 to 5b8134f Compare April 10, 2020 04:52

QuLogic reviewed Apr 10, 2020

View reviewed changes

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the bbox-docs branch from 1f0d625 to b31c241 Compare April 10, 2020 08:00

timhoffm reviewed Apr 10, 2020

View reviewed changes

timhoffm added the Documentation label Apr 10, 2020

timhoffm added this to the v3.3.0 milestone Apr 10, 2020

tacaswell approved these changes Apr 10, 2020

View reviewed changes

timhoffm reviewed Apr 10, 2020

View reviewed changes

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the bbox-docs branch from b31c241 to 7172b4c Compare April 11, 2020 04:49

timhoffm reviewed Apr 11, 2020

View reviewed changes

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the bbox-docs branch from 7172b4c to 14f3737 Compare April 11, 2020 18:21

timhoffm reviewed Apr 12, 2020

View reviewed changes

brunobeltran force-pushed the bbox-docs branch from 14f3737 to d09aa5a Compare April 12, 2020 02:05

timhoffm reviewed Apr 12, 2020

View reviewed changes

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

lib/matplotlib/transforms.py Outdated Show resolved Hide resolved

DOCS: add examples of how one "should" use Bbox

3f09493

brunobeltran force-pushed the bbox-docs branch from 5d29d66 to 3f09493 Compare April 13, 2020 02:57

timhoffm approved these changes Apr 13, 2020

View reviewed changes

timhoffm merged commit 6253d80 into matplotlib:master Apr 13, 2020

timhoffm mentioned this pull request Apr 13, 2020

Deprecate BBox.ignore() #17106

Closed

brunobeltran mentioned this pull request Apr 13, 2020

Proposal: BBoxs should be a a ring of sets #17115

Draft

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOCS: add examples of how one "should" use Bbox #17090

DOCS: add examples of how one "should" use Bbox #17090

brunobeltran commented Apr 10, 2020 •

edited

Loading

timhoffm Apr 10, 2020

brunobeltran Apr 10, 2020

brunobeltran Apr 10, 2020 •

edited

Loading

anntzer Apr 10, 2020

jklymak commented Apr 10, 2020

brunobeltran commented Apr 10, 2020

tacaswell commented Apr 10, 2020

tacaswell left a comment

timhoffm left a comment •

edited

Loading

brunobeltran commented Apr 11, 2020

anntzer commented Apr 11, 2020

timhoffm commented Apr 11, 2020

brunobeltran commented Apr 11, 2020

timhoffm left a comment

timhoffm Apr 12, 2020

brunobeltran Apr 12, 2020

timhoffm Apr 12, 2020

brunobeltran Apr 12, 2020

brunobeltran Apr 12, 2020

brunobeltran Apr 12, 2020

brunobeltran commented Apr 12, 2020

story645 commented Apr 12, 2020

brunobeltran commented Apr 13, 2020


		>>> Bbox.intersection(Bbox.null(), Bbox.null())
		Bbox([[-inf, -inf], [inf, inf]])


		Create from collections of points

		The "empty" object for accumulating Bboxs is the null bbox, which has

DOCS: add examples of how one "should" use Bbox #17090

DOCS: add examples of how one "should" use Bbox #17090

Conversation

brunobeltran commented Apr 10, 2020 • edited Loading

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunobeltran Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Apr 10, 2020

brunobeltran commented Apr 10, 2020

tacaswell commented Apr 10, 2020

tacaswell left a comment

Choose a reason for hiding this comment

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

brunobeltran commented Apr 11, 2020

anntzer commented Apr 11, 2020

timhoffm commented Apr 11, 2020

brunobeltran commented Apr 11, 2020

timhoffm left a 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

brunobeltran commented Apr 12, 2020

story645 commented Apr 12, 2020

brunobeltran commented Apr 13, 2020

brunobeltran commented Apr 10, 2020 •

edited

Loading

brunobeltran Apr 10, 2020 •

edited

Loading

timhoffm left a comment •

edited

Loading