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.

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

Fix for aspects of matplotlib LayoutPlot #826

Merged
merged 13 commits into from Aug 22, 2016

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Aug 21, 2016

One of the major current issues with the way Layouts are handled is that the code does not take into account non-square aspects when they have not been explicitly declared (via the aspect plot option). This means that any plots that use 'equal' aspect can leave significant white space in a layout figure.

The algorithm to resolve this problem goes as follows:

  1. Recursively iterate over all axes in the figure until they have been resolved into individual rows and columns, by their vertical and horizontal overlap.
  2. Iterate over the axes in each row and column and get their extents (including ticks and title).
  3. Use the maximum row width and maximum column height to compute the aspect of the plot and then set figure inches to respect that.
  4. If a layout title is defined, reposition it to the top of the figure.

This results in significantly improved default behavior for plots with non-square aspects because unnecessary white space is removed. For now I've exposed this via a fix_aspect plot option but this was mostly for testing, we can consider enabling it by default, but I'd have to do some further testing first.

It's worth noting that for very extreme aspects it might not get it completely right but rather than having to set a custom fig_inches the vspace and hspace will now give you much better control adjusting the spacing between plots.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 22, 2016

Okay this should now be ready to merge. We have decided not to enable the new behavior by default yet and wait until the v1.7 to do so. For now the new behavior can be enabled with:

from holoviews.plotting.mpl import LayoutPlot
LayoutPlot.v17_layout_format = True
LayoutPlot.vspace = 0.3
@jbednar

This comment has been minimized.

Member

jbednar commented Aug 22, 2016

Ok, sounds good. I'll try to enable that in all my notebooks so that it gets more testing.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 22, 2016

Looking forward to the improved behavior. The plan is to remove the class attribute v17_layout_format in 1.7 and replace it with pre_v17_layout_format for backwards compatibility when we release 1.7. Merging.

@jlstevens jlstevens merged commit 20877f5 into master Aug 22, 2016

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 46.496%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Aug 22, 2016

@jbednar

This comment has been minimized.

Member

jbednar commented Aug 22, 2016

Don't we have to keep v17_layout_format defined, and just make it a no-op, if we want user code to keep working? Otherwise people who want that behavior now will find their notebooks broken when they upgrade.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 22, 2016

Don't we have to keep v17_layout_format defined, and just make it a no-op, if we want user code to keep working? Otherwise people who want that behavior now will find their notebooks broken when they upgrade.

They'll get a warning that they're setting an undefined parameter but that's it, is that an issue?

@jbednar

This comment has been minimized.

Member

jbednar commented Aug 22, 2016

Ah, that's not so bad. But more helpful would be for it to print a warning saying that this setting is no longer needed as it's now the default. Still, as we'd eventually want to remove that warning, maybe the undefined parameter warning is fine, then.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 22, 2016

They'll get a warning that they're setting an undefined parameter ...

You mean class attribute I presume...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 22, 2016

You mean class attribute I presume...

No, param will complain you're trying to set an undefined parameter.

@jbednar jbednar deleted the fix_aspect branch Aug 22, 2016

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 22, 2016

Ah right.

Param assumes you are trying to set a parameter when really it was always just a class attribute. Just two ways of looking at it I guess...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment