Skip to content
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

Added option for dimension range padding #2293

Merged
merged 48 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@philippjfr
Copy link
Contributor

philippjfr commented Feb 1, 2018

Adds support for dimension range padding as suggested in #1090. Was reasonably straightforward, for backward compatibility no padding is added by default. Logic for computing padding on Overlays not yet quite correct.

All changes should for now be backward compatible and tests should pass.

@philippjfr philippjfr force-pushed the range_padding branch 3 times, most recently from 44df78d to 0423917 Feb 1, 2018

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Feb 1, 2018

Thanks! Looks like annoying code to have to write. :-) Can you show examples of how to turn it on, and e.g. how it will apply only to non-raster types?

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 1, 2018

Very happy to see this PR and I think having padding=0 makes sense for now - the tests should still pass for that value. I would like to echo what Jim said of showing some examples, ideally in the docs.

What I worry about is that it could potentially affect everything so I just hope we can get in enough testing before the next release. The important thing is that padding=0 works as before but having the ability to set padding too is going to be really valuable!

If all goes well, we might want to set a non-zero padding in this release (at least for some non-raster plot types) and a config option for backwards compatibility.

@philippjfr philippjfr force-pushed the range_padding branch from 0423917 to 830f1a3 Feb 1, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 1, 2018

For now I'm focused on getting tests passing, then I'll look at the API again (I suspect xpadding and ypadding are needed/useful). Don't think I'm in favor of enabling it by default for this release, it's too large a change in behavior to turn on by default. I think it should wait for 2.0 or at a minimum we should test it out for one release cycle before committing to default padding.

@philippjfr philippjfr force-pushed the range_padding branch from 830f1a3 to eefa767 Feb 1, 2018

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 1, 2018

... we should test it out for one release cycle before committing to default padding.

I think this option is the most reasonable then.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 4, 2018

I think we can probably get this into the 1.11 release. One thing I'm wondering is whether we should add xpadding and ypadding options or whether padding could optionally accept a tuple. Any thoughts @jbednar and @jlstevens?

@philippjfr philippjfr force-pushed the range_padding branch 2 times, most recently from 71cf6d9 to 5a9d550 Jul 4, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 4, 2018

This is now working as expected (the few test failures are expected and inconsequential) so before I start adding tests and documentation the API needs to be sorted out. The padding option clashes with one existing usage of padding on the matplotlib BarPlot, but since that needs a full rewrite anyway to be made consistent with the bokeh implementation I don't think that's a major issue. We do have to decide whether to add xpadding and ypadding, allow a tuple on padding or something else though.

@@ -597,6 +610,9 @@ class GenericElementPlot(DimensionedPlot):
logy = param.Boolean(default=False, doc="""
Whether the y-axis of the plot will be a log axis.""")

padding = param.Number(default=0, doc="""
Amount of padding to apply to data ranges.""")

This comment has been minimized.

@jbednar

jbednar Jul 5, 2018

Contributor

Some more detail would be good here, at least explaining that it's fractional and whether it is applied only to auto-computed ranges, or also to user-provided ranges. If the former, maybe "Amount of padding to apply to auto-computed data ranges, as a fraction of the autocomputed range. E.g. a value of 1.2 will result in a range 20% larger than the auto-computed range."?

@jbednar
Copy link
Contributor

jbednar left a comment

Looks good, but still needs a section in the docs showing how to use it before the API can be settled -- API can only be decided based on how it's used in practice.

From what I can tell, the value is a fraction applied to the current range? If that's true, then it won't depend on the axis scales, so I favor the tuple option for setting the padding per axis when needed. (If it was dependent on the axis scale, then I'd favor having separate settings, as the values would then be in separate units.)

Also, I'm not sure the current defaults will make sense for extreme aspect ratios. If e.g. x_range==10*y_range, it seems like a setting of 1.2 will result in a much larger absolute padding length on x than on y, which I think will look very bad. When you add docs, can you be sure to include an extreme aspect ratio like this? It might be best for the default to compute the value on longer axis, and then apply the same padding to both axes?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 5, 2018

From what I can tell, the value is a fraction applied to the current range?

Yes, if you see the linked issue that's what we had discussed. Matplotlib, bokeh, ggplot all use a default value of 5% padding, which I would express as 0.05 rather than 1.05.

If that's true, then it won't depend on the axis scales, so I favor the tuple option for setting the padding per axis when needed.

Makes sense.

Also, I'm not sure the current defaults will make sense for extreme aspect ratios. If e.g. x_range==10*y_range, it seems like a setting of 1.2 will result in a much larger absolute padding length on x than on y, which I think will look very bad.

I don't understand this, if it's a fraction then a value of 1.2 or rather 0.2 will look visually the same whatever the aspect ratio of the scales is:

screen shot 2018-07-05 at 11 42 21 am

@ea42gh

This comment has been minimized.

Copy link
Contributor

ea42gh commented Jul 5, 2018

This has been on my wishlist for quite a while!

re tuple option, how would you handle twin axes?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 5, 2018

re tuple option, how would you handle twin axes?

We currently have no real support for twin axes, but it is worth keeping in mind I suppose. Neither the xpadding/ypadding or the padding approach has any serious advantage for that case. In the vast majority of cases one value for both axes would be sufficient as well. If you have any suggestions that would make separate values for twin axes easier I'd love to hear them but atm I see no strong reason to change the design on account of them.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jul 5, 2018

0.2 will look visually the same whatever the aspect ratio of the scales is:

Sorry to be unclear -- I wasn't talking about the aspect ratio of the scales, but of the plot itself. I.e. I should have said width and height, not x_range and y_range, and that we need an example where e.g. height=width/10. Seems like we'd want the same padding on both axes in such a case, yet if I understand it correctly the current implementation will give 10x the padding in width than in height instead.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 5, 2018

I see, yes that is true but I don't really consider it a major issue. If you do that you'll be able to set different padding values using the tuple format. Also I would consider extreme aspect ratios (>5) pretty rare cases. Here is an example with an aspect ratio of 8:

screen shot 2018-07-05 at 2 21 18 pm

As far as I'm aware other libraries do not provide any special handling for the padding if the aspect isn't square so I think my vote is not to overcomplicate things and make the padding dependent on the aspect.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jul 5, 2018

Well, the extreme case just makes it clear what's going on, which is that the padding won't really be appropriate for anything but square aspects. I don't think it's anything to do with overcomplicating things, I think it's that currently the padding is being done with respect to entirely the wrong quantities, which becomes evident only when looking at an appropriate test case like you just showed. Sure, for square aspects it's ok, but don't you think that when people want padding, they want approximately the same padding around the whole plot, i.e. the same number of inches on the page or pixels on the screen? Your two examples above just make that clear -- a padding that's about the fraction of the plot range really isn't what people really want when they ask for padding. So why should we follow the incorrect example of some other library, if it only accidentally ever works? I.e. I'm not saying we should put in special handling for non-square aspects, I'm saying that the non-square aspect case reveals that the current logic is incorrect; there should be a calculation that works properly for both square and non-square aspects. Using the tuple format should only be for cases where users want to do something specifically unusual, not for compensating for a failure of our algorithm to do the right thing in the very common circumstance of not having perfectly square aspects. It's square aspects that are the special case; non-square aspects are the general case!

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jul 5, 2018

Basically you want the padding to occur in screen space, but recall once again that we do not have access to the actual plot dimensions ahead of time (in bokeh at least), which means that there is currently no real way to achieve that with accuracy.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jul 5, 2018

I agree with Jim's desire (do it in screen space) but I also agree with Philipp that there are technical hurdles why we can't currently do it that way (unfortunately!). We need something to address this problem which is why I support this PR even though I would also love to have screen space padding.

@ea42gh

This comment has been minimized.

Copy link
Contributor

ea42gh commented Jul 5, 2018

Here is a use case I have:
streaming data, non-negative x and y values.
I typically keep the axes centered on the origin, and pad the upper range of the x and y axes.

Looking forward to trying this out!

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jul 5, 2018

Ok, now the issue is clearer, thanks. Do we have no access to the screen space, or do we have a nominal width and height that may or may not be respected given space for the axes, labels, etc.? If we do have some nominal values, I would vote for using those values as being better than using a fraction of the data range; it seems like those values are nearly correct in general, while the fraction of data range is only correct in the special case of square aspects.

@philippjfr philippjfr force-pushed the range_padding branch from 96db735 to c50200a Sep 12, 2018

Defines the span of an axis if the axis range is zero, i.e. if
the lower and upper end of an axis are equal. For example if
there is a single datapoint at 0 a default_span of 2.0 will
result in axis ranges spanning from -1 to 1.""")

This comment has been minimized.

@jbednar

jbednar Sep 13, 2018

Contributor

The equivalent of default_span in datashader is used both when min==max and also when there is no data (e.g. if someone filtered the data dynamically and no datapoints were in the filter range). Is that true here? If so, presumably it should be mentioned in the docs.

This comment has been minimized.

@philippjfr

philippjfr Sep 13, 2018

Author Contributor

Sure, I can add that once we've decided on a name for the parameter.

This comment has been minimized.

@jbednar

jbednar Sep 13, 2018

Contributor

I don't mind the name. In datashader I used a default span of 1, i.e. -0.5, 0.5. Has the 2.0 value here ever been in a HoloViews release (by any name)? If so I can change datashader to match, as it's only been released with a value of 1.0 for a few days.

This comment has been minimized.

@philippjfr

philippjfr Sep 13, 2018

Author Contributor

Both the name and the default value of 2 were copied from bokeh. @jlstevens indicated he didn't like the name in a comment above but we both dislike his only suggestion even more:

I see. I would like to have a better name for this concept but I don't have one to suggest at the moment. Conceptually it is something like fallback_span but that name is horrible...

My vote is therefore to stick with default_span.

This comment has been minimized.

@jbednar

jbednar Sep 13, 2018

Contributor

Same here. Ok, I'll change datashader to 2.0.

Philipp Rudiger added some commits Sep 13, 2018

@philippjfr philippjfr force-pushed the range_padding branch from b691374 to 3c92124 Sep 13, 2018

Philipp Rudiger Philipp Rudiger
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Sep 13, 2018

Once tests pass this will be ready to merge. I've addressed the majority of the comments. It would be good to have an immediate dev release after this, so I can merge the corresponding changes into geoviews.

Philipp Rudiger Philipp Rudiger
@@ -150,6 +150,9 @@ class RasterGridPlot(GridPlot, OverlayPlot):
equivalent using subplots.
"""

padding = param.Number(default=0.1, doc="""
The amount of padding as a fraction of the total Grid size""")

This comment has been minimized.

@jlstevens

jlstevens Sep 13, 2018

Contributor

This padding parameter is a bit different from the common one (potentially confusing/clashing).

This comment has been minimized.

@philippjfr

philippjfr Sep 13, 2018

Author Contributor

Yes, thanks for catching that. It might cause confusion but I don't think it's a huge issue, it's on GridPlot and therefore won't actually clash, and also only exists in the matplotlib backend.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Sep 13, 2018

Looks good and I'm happy to merge (though I've made one new comment for you to think about first)...

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Sep 13, 2018

I think we can merge, while it might cause confusion the padding parameter on GridSpace does not actually clash with that on ElementPlot and also only applies to the matplotlib backend. I think I'll open a quick issue to rename the GridSpace parameter.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Sep 13, 2018

Actually I don't think we need to or should rename, padding on a GridPlot is a very similar concept.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Sep 13, 2018

Ok, sounds good! Merging.

@jlstevens jlstevens merged commit a1b9cde into master Sep 13, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 83.747%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr added this to the v1.11.0 milestone Nov 5, 2018

@philippjfr philippjfr deleted the range_padding branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.