Single-line string notation for subplot_mosaic #18763

timhoffm · 2020-10-17T22:31:48Z

PR Summary

This allows

plt.subplot_mosaic("AB;CC")

instead of the verbose

plt.subplot_mosaic(
    """
    AB
    CC
    """)

story645 · 2020-10-18T00:02:38Z

lib/matplotlib/figure.py

@@ -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:


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

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.

story645 · 2020-10-18T00:04:33Z

lib/matplotlib/tests/test_figure.py

@@ -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


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)

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.

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.

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.

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.

story645 · 2020-10-30T03:07:09Z

lib/matplotlib/tests/test_figure.py

@@ -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


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.

tacaswell · 2020-10-30T23:28:00Z

I just tried to use this and am sad it is not there. 'AB;CD' felt way better to type than 'AB\nCD' (which is super subjective but).

timhoffm · 2020-11-07T16:14:25Z

Ping @story645 #18763 (comment) please comment.

story645 · 2020-11-07T23:19:44Z

I continue to disagree w/ you and @tacaswell about the tests - having an overview of the different ways in which you broke something can lead to a faster solution than only learning about the different ways you broke something as you unbreak it.

And here as the parameterization is two values, I think it's basically as readable as the asserts, especially since the testing function has been renamed.

ETA: someone else is welcome to override me on this or argue that I can open up a PR to parameterize these tests myself.

anntzer

I think the nonparametrized form is just fine.

jklymak · 2020-11-08T02:16:05Z

I also think the tests are better not being parameterized. Parameterizing is great if you have a matrix of things you are testing, but otherwise I think they are needlessly hard to read. I'm not sure I understand the "overview of all the problems" idea - that sounds like a theoretical rather than practical concern.

jklymak · 2020-11-08T02:55:12Z

.. further, if you parameterize the tests, it is a lot more work to move the test to a separate script for debugging, which is something I do all the time to bypass all the py.test verbiage

story645 · 2020-11-08T05:36:39Z

@jklymak since the test fails on the first assert, you won't know if your code broke on the other types of input until you fix that one first. That means you have to go down the line individually when sometimes getting a sense of all the conditions on which it fails (or that it fails multiple conditions) is better for pinpointing what went wrong.

timhoffm · 2020-11-08T20:45:09Z

@story645 "getting a sense of all the conditions on which it fails" is nice in theory. However, in practice the pytest output is so unwieldy that it's hard to grasp that overview. To illustrate, I've introduced an error and posted the pytest logs for both variants below.

Additionally, often multiple tests fail for the same reason, in which case the multiple failures don't give additional insight and are only clutter. And if they fail for different reasons, often one has to address these reasons separately.

Non-parametrized test

============================================== FAILURES ===============================================
________________________________ TestSubplotMosaic.test_string_parser _________________________________

self = <matplotlib.tests.test_figure.TestSubplotMosaic object at 0x7f1ba361d650>

    def test_string_parser(self):
        normalize = Figure._normalize_grid_string
>       assert normalize('ABC') == [['A', 'B', 'C']]
E       AssertionError: assert ['ABC'] == [['A', 'B', 'C']]
E         At index 0 diff: 'ABC' != ['A', 'B', 'C']
E         Use -v to get the full diff

lib/matplotlib/tests/test_figure.py:783: AssertionError

Parametrized test

_______________________ TestSubplotMosaic.test_string_parser2[ABC-list_layout0] _______________________

self = <matplotlib.tests.test_figure.TestSubplotMosaic object at 0x7f478fc686d0>, str_layout = 'ABC'
list_layout = [['A', 'B', 'C']]

    @pytest.mark.parametrize('str_layout, list_layout', [
        ('ABC', [['A', 'B', 'C']]),
        ('AB;CC',  [['A', 'B'], ['C', 'C']]),
        ('AB;CC;DE', [['A', 'B'], ['C', 'C'], ['D', 'E']]),
        ("""
         ABC
         """, [['A', 'B', 'C']]),
        ("""
         AB
         CC
         """, [['A', 'B'], ['C', 'C']]),
        ("""
         AB
         CC
         DE
         """, [['A', 'B'], ['C', 'C'], ['D', 'E']])
    ])
    def test_string_parser2(self, str_layout, list_layout):
>       assert Figure._normalize_grid_string(str_layout) == list_layout
E       AssertionError: assert ['ABC'] == [['A', 'B', 'C']]
E         At index 0 diff: 'ABC' != ['A', 'B', 'C']
E         Use -v to get the full diff

lib/matplotlib/tests/test_figure.py:779: AssertionError
______________________ TestSubplotMosaic.test_string_parser2[AB;CC-list_layout1] ______________________

self = <matplotlib.tests.test_figure.TestSubplotMosaic object at 0x7f478fc00710>, str_layout = 'AB;CC'
list_layout = [['A', 'B'], ['C', 'C']]

    @pytest.mark.parametrize('str_layout, list_layout', [
        ('ABC', [['A', 'B', 'C']]),
        ('AB;CC',  [['A', 'B'], ['C', 'C']]),
        ('AB;CC;DE', [['A', 'B'], ['C', 'C'], ['D', 'E']]),
        ("""
         ABC
         """, [['A', 'B', 'C']]),
        ("""
         AB
         CC
         """, [['A', 'B'], ['C', 'C']]),
        ("""
         AB
         CC
         DE
         """, [['A', 'B'], ['C', 'C'], ['D', 'E']])
    ])
    def test_string_parser2(self, str_layout, list_layout):
>       assert Figure._normalize_grid_string(str_layout) == list_layout
E       AssertionError: assert ['AB', 'CC'] == [['A', 'B'], ['C', 'C']]
E         At index 0 diff: 'AB' != ['A', 'B']
E         Use -v to get the full diff

lib/matplotlib/tests/test_figure.py:779: AssertionError
____________________ TestSubplotMosaic.test_string_parser2[AB;CC;DE-list_layout2] _____________________

self = <matplotlib.tests.test_figure.TestSubplotMosaic object at 0x7f478fccd750>
str_layout = 'AB;CC;DE', list_layout = [['A', 'B'], ['C', 'C'], ['D', 'E']]

    @pytest.mark.parametrize('str_layout, list_layout', [
        ('ABC', [['A', 'B', 'C']]),
        ('AB;CC',  [['A', 'B'], ['C', 'C']]),
        ('AB;CC;DE', [['A', 'B'], ['C', 'C'], ['D', 'E']]),
        ("""
         ABC
         """, [['A', 'B', 'C']]),
        ("""
         AB
         CC
         """, [['A', 'B'], ['C', 'C']]),
        ("""
         AB
         CC
         DE
         """, [['A', 'B'], ['C', 'C'], ['D', 'E']])
    ])
    def test_string_parser2(self, str_layout, list_layout):
>       assert Figure._normalize_grid_string(str_layout) == list_layout
E       AssertionError: assert ['AB', 'CC', 'DE'] == [['A', 'B'], ...], ['D', 'E']]
E         At index 0 diff: 'AB' != ['A', 'B']
E         Use -v to get the full diff

lib/matplotlib/tests/test_figure.py:779: AssertionError

story645

Ok, even I don't want to be blocking on stuff I think should be addressed in a later PR.

timhoffm force-pushed the subplot_mosaic-compact branch from 4c48f7e to 469cfbe Compare October 17, 2020 22:39

Single-line string notation for subplot_mosaic

235165b

timhoffm force-pushed the subplot_mosaic-compact branch from 469cfbe to 235165b Compare October 17, 2020 22:46

story645 requested changes Oct 18, 2020

View reviewed changes

tacaswell approved these changes Oct 26, 2020

View reviewed changes

tacaswell added this to the v3.4.0 milestone Oct 26, 2020

jklymak requested a review from story645 October 26, 2020 20:40

story645 requested changes Oct 30, 2020

View reviewed changes

timhoffm added the status: needs review label Nov 3, 2020

timhoffm mentioned this pull request Nov 6, 2020

DOC: remove examples of subplot(123) from docs #17335

Closed

6 tasks

anntzer approved these changes Nov 8, 2020

View reviewed changes

story645 approved these changes Nov 9, 2020

View reviewed changes

story645 removed the status: needs review label Nov 9, 2020

story645 merged commit 93157a3 into matplotlib:master Nov 9, 2020

timhoffm deleted the subplot_mosaic-compact branch November 9, 2020 20:00

QuLogic mentioned this pull request Mar 18, 2021

DOC: Prepare What's new page for 3.4.0. #19713

Merged

5 tasks

story645 mentioned this pull request Apr 27, 2022

"ab;cd" missing in subplot_mosaic tutorial #22917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single-line string notation for subplot_mosaic #18763

Single-line string notation for subplot_mosaic #18763

timhoffm commented Oct 17, 2020 •

edited

Loading

story645 Oct 18, 2020

timhoffm Oct 18, 2020

story645 Oct 18, 2020

timhoffm Oct 18, 2020 •

edited

Loading

story645 Oct 30, 2020

timhoffm Oct 30, 2020 •

edited

Loading

tacaswell Oct 30, 2020

story645 Oct 30, 2020

tacaswell commented Oct 30, 2020

timhoffm commented Nov 7, 2020

story645 commented Nov 7, 2020 •

edited

Loading

anntzer left a comment

jklymak commented Nov 8, 2020

jklymak commented Nov 8, 2020

story645 commented Nov 8, 2020

timhoffm commented Nov 8, 2020 •

edited

Loading

story645 left a comment •

edited

Loading

Single-line string notation for subplot_mosaic #18763

Single-line string notation for subplot_mosaic #18763

Conversation

timhoffm commented Oct 17, 2020 • edited Loading

PR Summary

story645 Oct 18, 2020

Choose a reason for hiding this comment

timhoffm Oct 18, 2020

Choose a reason for hiding this comment

story645 Oct 18, 2020

Choose a reason for hiding this comment

timhoffm Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

story645 Oct 30, 2020

Choose a reason for hiding this comment

timhoffm Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

tacaswell Oct 30, 2020

Choose a reason for hiding this comment

story645 Oct 30, 2020

Choose a reason for hiding this comment

tacaswell commented Oct 30, 2020

timhoffm commented Nov 7, 2020

story645 commented Nov 7, 2020 • edited Loading

anntzer left a comment

Choose a reason for hiding this comment

jklymak commented Nov 8, 2020

jklymak commented Nov 8, 2020

story645 commented Nov 8, 2020

timhoffm commented Nov 8, 2020 • edited Loading

Non-parametrized test

Parametrized test

story645 left a comment • edited Loading

Choose a reason for hiding this comment

timhoffm commented Oct 17, 2020 •

edited

Loading

timhoffm Oct 18, 2020 •

edited

Loading

timhoffm Oct 30, 2020 •

edited

Loading

story645 commented Nov 7, 2020 •

edited

Loading

timhoffm commented Nov 8, 2020 •

edited

Loading

story645 left a comment •

edited

Loading