Skip to content

Single-line string notation for subplot_mosaic #18763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Single-line string notation for subplot_mosaic #18763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions lib/matplotlib/figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,8 +1776,13 @@ def get_tightbbox(self, renderer, bbox_extra_artists=None):

@staticmethod
def _normalize_grid_string(layout):
layout = inspect.cleandoc(layout)
return [list(ln) for ln in layout.strip('\n').split('\n')]
if '\n' not in layout:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do if ';' in layout? Should ';\nA' be valid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've designed this PR to not change the behavior of multi-line strings. I've just added the possibility of single-line strings. Therefore the check is between those two options ("Do I have a line break?").

Whether or not we want stronger checking for the multi-line case is orthogonal.

# single-line string
return [list(ln) for ln in layout.split(';')]
else:
# multi-line string
layout = inspect.cleandoc(layout)
return [list(ln) for ln in layout.strip('\n').split('\n')]

def subplot_mosaic(self, layout, *, subplot_kw=None, gridspec_kw=None,
empty_sentinel='.'):
Expand Down Expand Up @@ -1812,16 +1817,21 @@ def subplot_mosaic(self, layout, *, subplot_kw=None, gridspec_kw=None,
Any of the entries in the layout can be a list of lists
of the same form to create nested layouts.

If input is a str, then it must be of the form ::
If input is a str, then it can either be a multi-line string of
the form ::

'''
AAE
C.E
'''

where each character is a column and each line is a row.
This only allows only single character Axes labels and does
not allow nesting but is very terse.
where each character is a column and each line is a row. Or it
can be a single-line string where rows are separated by ``;``::

'AB;CC'

The string notation allows only single character Axes labels and
does not support nesting but is very terse.

subplot_kw : dict, optional
Dictionary with keywords passed to the `.Figure.add_subplot` call
Expand Down
18 changes: 18 additions & 0 deletions lib/matplotlib/tests/test_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,24 @@ def test_subplot_kw(self, fig_test, fig_ref, subplot_kw):

axB = fig_ref.add_subplot(gs[0, 1], **subplot_kw)

def test_string_parser(self):
normalize = Figure._normalize_grid_string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should possibly be paramterized instead of reassigning the function name if the point of the reassignment is to not have to write the long name (& also these tests are really testing different conditions)

Copy link
Member Author

@timhoffm timhoffm Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you strictly go by the book, yes all these assertions could be separate tests and thus be parametrized. However, from a practical point of view, an explicit assert normalize('ABC') == [['A', 'B', 'C']] is much more readable than

@pytest.mark.parametrize('str_layout, list_layout', [
    ('ABC', [['A', 'B', 'C']]),
    ...
])
test_string_parser(str_layout, list_layout):
     assert Figure._normalize_grid_string(str_layout) == list_layout

And for readability, I'll accept shortening the function name and doing all assertions in one test. IMHO that's still better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure test output readability is better though? (I don't know what the output looks like here.

Also my concern isn't readability so much as there's no way to see that multiple conditions fail since by definition the first failed assert will fail the test.

Copy link
Member Author

@timhoffm timhoffm Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test passes there is no output. In case of failure, you get a traceback including a printout of the failed assert statement. IMHO the assert is a bit more readable with the explicit strings, compared to using variables and resolving them ("... where str_layout = ... and list_layout = ...").

But actually I'm more concerned with the readability of the test. The tests are validated examples. They define the API we actually guarantee (docstrings can claim anything). Thus, they are a source of information and should be easily readable.

You are right with the multiple conditions. It's very well possible that multiple assertions fail if someone messes up the parser. However, I believe that knowing multiple asserts failed does not help for debugging in this context. Pytest will tell you the number of broken tests. If there is one, you go there and immediately see "My change broke the mosaic parser". If you have 3 failed tests, you have to check all three to see that you only broke the parser but nothing else. This is significantly more cumbersome. IMHO the added information "My change broke the mosaic parser in three ways" does not help much. In practice, you will anyway go to the first failed test and try to make that work again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @timhoffm this is is more readable an informative than what it would look like with @pytest.mark.parameterize and that splitting this out into N tests would not provide much benefit.

assert normalize('ABC') == [['A', 'B', 'C']]
assert normalize('AB;CC') == [['A', 'B'], ['C', 'C']]
assert normalize('AB;CC;DE') == [['A', 'B'], ['C', 'C'], ['D', 'E']]
assert normalize("""
ABC
""") == [['A', 'B', 'C']]
assert normalize("""
AB
CC
""") == [['A', 'B'], ['C', 'C']]
assert normalize("""
AB
CC
DE
""") == [['A', 'B'], ['C', 'C'], ['D', 'E']]

@check_figures_equal(extensions=["png"])
@pytest.mark.parametrize("str_pattern",
["AAA\nBBB", "\nAAA\nBBB\n", "ABC\nDEF"]
Expand Down