Skip to content

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 1, 2020

This avoids having to check for ratios == None every time someone
actually accesses the ratios.

I just deleted the never used and private vstackeq and hstackeq
instead of changing their signatures to make height_ratios/width_ratios
non-optional, given that they should be simple enough to bring back if
needed.

PR Summary

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

@anntzer anntzer added topic: geometry manager LayoutEngine, Constrained layout, Tight layout Maintenance labels May 1, 2020
@jklymak
Copy link
Member

jklymak commented May 1, 2020

.. though I guess the test failure is real?

@jklymak
Copy link
Member

jklymak commented May 1, 2020

at some point I figured out how to look at the failing images on CI, but I cannot figure it out again :-(

@tacaswell tacaswell added this to the v3.4.0 milestone May 1, 2020
@QuLogic
Copy link
Member

QuLogic commented May 2, 2020

Artifacts are here

@@ -49,9 +49,9 @@ def __init__(self, nrows, ncols, height_ratios=None, width_ratios=None):

def __repr__(self):
height_arg = (', height_ratios=%r' % (self._row_height_ratios,)
if self._row_height_ratios is not None else '')
if len({*self._row_height_ratios}) != 1 else '')
Copy link
Member

Choose a reason for hiding this comment

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

Slightly prefer

Suggested change
if len({*self._row_height_ratios}) != 1 else '')
if len(set(self._row_height_ratios)) != 1 else '')

for better readability (also below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I already won this battle: there's multiple occurrences of {*foo} in the codebase already and none of set(*foo) :-)

Copy link
Member

Choose a reason for hiding this comment

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

Um, set(*foo) is not valid. It's set(foo) because set() takes a single iterable as argument.

That's part of why I prefer it here: "make as set from this list" is just a single concept, whereas "unpack this iterable and create a set from its elements" is two concepts. (I don't have that much brain capacity https://youtu.be/UANN2Eu6ZnM 😄).

Yes, you've already sneaked 5 times {*foo} into the code base. - I hope I didn't approve these 😝.

But for real, {*a, *b} makes sense for multiple iterables, but for a single iterable set(a) is more readable than {*a}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, changed to set().

@anntzer
Copy link
Contributor Author

anntzer commented May 2, 2020

Actually that looks like a real failure, see #17304.

@anntzer
Copy link
Contributor Author

anntzer commented May 2, 2020

Currently blocked by #17304.

@anntzer anntzer marked this pull request as draft May 2, 2020 15:58
@anntzer anntzer force-pushed the ratios branch 2 times, most recently from fb8ddd1 to 583340e Compare May 2, 2020 19:38
@anntzer anntzer marked this pull request as ready for review May 2, 2020 19:38
@anntzer
Copy link
Contributor Author

anntzer commented May 2, 2020

hopefully working now

@anntzer
Copy link
Contributor Author

anntzer commented May 2, 2020

Looks like another failure in test_pickle, but that one already has a nonzero tolerance for certain architectures which suggests fp issues so I don't feel bad putting in the same tolerance for x86_64.

@anntzer
Copy link
Contributor Author

anntzer commented May 3, 2020

And another test is failing only on CI with a tiny delta (I can't even repro it locally), so I just upped the tol. The baseline and results are basically impossible to differentiate visually,
tight_layout4-expected_svg
tight_layout4_svg
the diff is
tight_layout4_svg-failed-diff

This avoids having to check for `ratios == None` every time someone
actually accesses the ratios.

I just deleted the never used and private `vstackeq` and `hstackeq`
instead of changing their signatures to make height_ratios/width_ratios
non-optional, given that they should be simple enough to bring back if
needed.
@timhoffm timhoffm merged commit 5f84e79 into matplotlib:master Aug 18, 2020
@anntzer anntzer deleted the ratios branch August 18, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants