Improve table docs #12218

timhoffm · 2018-09-22T21:32:52Z

PR Summary

Changes:

Documented Cell.
Rewrote the module docstring.

Do we need the author, copyright, license stuff in the module docstring? Or are the redundant, because this stuff is handled on the project-level?

I could not make sense of the following two sentences in the module docstring. Therefore I removed that information. If anyone knows what these should mean, I'd be happy to add a more clear description.

The grid need not be rectangular

You can add additional cells outside this [(max_row, max_col)] range to have convenient
ways of positioning more interesting grids.

anntzer · 2018-09-22T22:06:49Z

See 735f308#diff-3ca3e0d4e31b414f1e771501e9bf69d3L3 for how I handled authorship issues. I have no problem with leaving in who originally contributed the module, but they're obviously not the sole author anymore.

ImportanceOfBeingErnest · 2018-09-23T01:07:58Z

I interprete the sentence

The grid need not be rectangular. You can add additional cells outside this [(max_row, max_col)] range to have convenient ways of positioning more interesting grids.

such that you are not restricted to the initial grid of the table. E.g. if you created a 11x2 table, you may still add cells outside the range of [0,1] columns and [0,10] rows. E.g.

Code

import matplotlib.pyplot as plt

fig, ax = plt.subplots()

c1 = ["pale", "dark"]
c2 = ["green", "turquoise"]
c3 = ["crimson", "bisque"]

a = [["".join((c1[i%2],c2[(j+i)%2])) for i in range(2)] for j in range(11)]

table = ax.table(cellColours=a, loc="center left", colWidths=[0.05]*2)
h = table.get_celld()[(0,0)].get_height()
for j in range(5):
    for i in range(1+j,10-j):
        table.add_cell(i,j+2, 0.05, h, facecolor=c3[(i+j)%2])
        table.add_cell(i,10-j, 0.05, h, facecolor=c3[(i+j)%2])
        table.add_cell(12-j,i+1, 0.05, h, facecolor=c3[(i-j-1)%2])
    table.add_cell(-3,j+4, 0.05, h, facecolor="g"+["","old"][j%2])
        
plt.show()

timhoffm · 2018-09-23T10:12:20Z

@ImportanceOfBeingErnest Thanks, I've added that information to the Table class docstring.

timhoffm · 2018-09-23T10:34:52Z

See 735f308#diff-3ca3e0d4e31b414f1e771501e9bf69d3L3 for how I handled authorship issues. I have no problem with leaving in who originally contributed the module, but they're obviously not the sole author anymore.

@anntzer Thanks, I've now used that style and converted author and copyright there. I've removed the license, because it just says "matplotlib license", which is the license of the whole project anyway. Not sure if we have to keep the copyright notice.

anntzer · 2018-09-23T10:53:27Z

No opinion there; @tacaswell?

jklymak

Seems fine. Not sure about the author/copyright stuff either - we definitely don't do this anymore.

anntzer · 2018-10-06T22:17:27Z

Letting @tacaswell have a look at the copyright issue, if he cares.

timhoffm · 2018-10-13T16:47:50Z

@tacaswell explicit question: What to do with author/copyright notices in docstrings:

https://github.com/matplotlib/matplotlib/pull/12218/files#diff-cd21097aa8cce79ca8a23b7f52c48f99

Keep them in the docstring
Move them from the docstring to a comment in the file, so that they do not show up in the docs but are still in the source code
Remove them because they are not necessary in that context

tacaswell · 2018-10-14T05:07:10Z

I do not think we can remove them.

I think the best path forward is to add "Original code by:" above it and "subsequent changes are copyright Matplotlib Developers" below, and to move it out of the docstring.

tacaswell

copyright changes, anyone can dismiss this.

Copyright notice adapted.

timhoffm · 2018-11-01T21:31:33Z

Rebased.

anntzer · 2018-11-02T10:39:52Z

lib/matplotlib/table.py

-    PAD = 0.1  # padding between text and rectangle
+
+    PAD = 0.1
+    """Padding between text and rectangle."""


Kind of prefer leaving this as inline comment (fwiw).
(As a side note, compare with the padding of Annotations, which is also hardcoded

pad = renderer.points_to_pixels(4)

)

Since this is a public attribute, it shows up in the docs

I very much prefer that every element in the docs has a description. This is achived using the docstring

I would be fine not documenting PAD, but only if we also remove it from the docs (either by excluding explicitly (not sure if that's feasible with autodoc in this context), or preferably by renaming to _PAD.

Oh, I didn't realize sphinx would pick it up. I guess that works, then...

anntzer · 2018-11-02T10:40:57Z

lib/matplotlib/table.py

@@ -42,7 +71,7 @@ def __init__(self, xy, width, height,
                 ):

        # Call base
-        Rectangle.__init__(self, xy, width=width, height=height,
+        Rectangle.__init__(self, xy, width=width, height=height, fill=fill,


I guess that's an intended new feature (respecting fill instead of dropping it)?.

Yes. I assume that is was accidentally omitted. There's no point in having fill in __init__ if it's silently dropped.

anntzer · 2018-11-02T10:41:49Z

lib/matplotlib/table.py

@@ -124,18 +154,27 @@ def _set_text_position(self, renderer):
        self._text.set_position((x, y))

    def get_text_bounds(self, renderer):
-        """ Get text bounds in axes co-ordinates. """
+        """
+        Return the bounds of the text as *(x, y, width, height)* in table


"Return text bounds etc." fits in one line.

anntzer · 2018-11-02T10:43:37Z

lib/matplotlib/table.py

+    The table consists of a grid of cells, which are indexed by (row, column).
+
+    For a simple table, you'll have a full grid of cells with indices from
+    (0, 0) to (max_rows-1, max_cols-1), in which the cell (0, 0) is positioned


(n_rows-1, n_cols-1) or (max_row, max_col) (by definition max_foo is the largest value of foo...).

anntzer · 2018-11-02T19:14:24Z

lib/matplotlib/table.py

@@ -29,9 +29,38 @@

 class Cell(Rectangle):
    """
-    A cell is a  `.Rectangle` with some associated text.
+    A cell is a  `.Rectangle` with some associated `.Text`.


extra space after "is a"

lumberbot-app · 2018-11-22T11:07:16Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.0.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 3c702c495d60c41e940855d1e80d9de3cd75c49e

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #12218: Improve table docs'

Push to a named branch :

git push YOURFORK v3.0.x:auto-backport-of-pr-12218-on-v3.0.x

Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #12218 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

lumberbot-app · 2018-11-22T11:07:48Z

Something went wrong ... Please have a look at my logs.

It seem that the branch you are trying to backport to does not exists.

anntzer · 2018-11-22T11:08:42Z

Sorry, didn't pay attention to the milestone. Made it 3.1 as it touches .py sources (not against someone doing a manual backport to 3.0.x but doesn't seem urgent either).

timhoffm added the Documentation label Sep 22, 2018

timhoffm added this to the v3.0.x milestone Sep 22, 2018

timhoffm force-pushed the doc-table branch from b822575 to 73db1ff Compare September 22, 2018 21:44

timhoffm force-pushed the doc-table branch from 73db1ff to e8aeab0 Compare September 23, 2018 10:11

timhoffm force-pushed the doc-table branch from e8aeab0 to 1c28034 Compare September 23, 2018 10:27

jklymak modified the milestones: v3.0.x, v3.0.0-doc Oct 3, 2018

jklymak approved these changes Oct 3, 2018

View reviewed changes

timhoffm added the status: needs review label Oct 6, 2018

anntzer assigned tacaswell Oct 6, 2018

tacaswell previously requested changes Oct 14, 2018

View reviewed changes

timhoffm force-pushed the doc-table branch from 1c28034 to 5afe234 Compare October 14, 2018 08:42

timhoffm force-pushed the doc-table branch from 5afe234 to 801b58e Compare November 1, 2018 21:31

anntzer reviewed Nov 2, 2018

View reviewed changes

timhoffm force-pushed the doc-table branch from 801b58e to f0967e1 Compare November 2, 2018 19:03

anntzer reviewed Nov 2, 2018

View reviewed changes

Improve table docs

ba4ad6d

timhoffm force-pushed the doc-table branch from f0967e1 to ba4ad6d Compare November 2, 2018 20:20

anntzer approved these changes Nov 22, 2018

View reviewed changes

anntzer removed the status: needs review label Nov 22, 2018

anntzer merged commit 3c702c4 into matplotlib:master Nov 22, 2018

lumberbot-app bot added the status: needs manual backport label Nov 22, 2018

anntzer modified the milestones: v3.0.0-doc, v3.1 Nov 22, 2018

anntzer removed the status: needs manual backport label Nov 22, 2018

timhoffm deleted the doc-table branch November 22, 2018 21:31

timhoffm mentioned this pull request Nov 24, 2018

More table documentation #12880

Merged

Uh oh!

Improve table docs #12218

Improve table docs #12218

Uh oh!

Conversation

timhoffm commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

anntzer commented Sep 22, 2018

Uh oh!

ImportanceOfBeingErnest commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timhoffm commented Sep 23, 2018

Uh oh!

timhoffm commented Sep 23, 2018

Uh oh!

anntzer commented Sep 23, 2018

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Oct 6, 2018

Uh oh!

timhoffm commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Nov 1, 2018

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

timhoffm Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

timhoffm Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

anntzer Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

lumberbot-app bot commented Nov 22, 2018

Uh oh!

lumberbot-app bot commented Nov 22, 2018

Uh oh!

anntzer commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

timhoffm commented Sep 22, 2018 •

edited

Loading

ImportanceOfBeingErnest commented Sep 23, 2018 •

edited

Loading

timhoffm commented Oct 13, 2018 •

edited

Loading

tacaswell commented Oct 14, 2018 •

edited

Loading

timhoffm Nov 2, 2018 •

edited

Loading

anntzer commented Nov 22, 2018 •

edited

Loading