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

Enable padding on all non-raster elements #4182

Merged
merged 20 commits into from Jan 15, 2020
Merged

Enable padding on all non-raster elements #4182

merged 20 commits into from Jan 15, 2020

Conversation

@philippjfr
Copy link
Member

philippjfr commented Jan 14, 2020

  • Fixes #1090
  • Set padding default to 0.1, default for Area/Curve/Spread to (0, 0.1) and default for raster-like elements to 0
  • Update tests
  • Remove explicit padding from examples
  • Fixed shared axes in plotly
  • Fixed BarPlot get_extents issues
  • Add config.no_padding flag
@philippjfr philippjfr force-pushed the enable_padding branch from 15576e3 to 08b13c4 Jan 14, 2020
@jbednar

This comment has been minimized.

Copy link
Member

jbednar commented Jan 14, 2020

Non raster elements don't seem to share a base class?

@philippjfr philippjfr force-pushed the enable_padding branch from 08b13c4 to c1faf99 Jan 14, 2020
@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Jan 14, 2020

No, they generally don't share any implementation and/or parameters. Might make sense to introduce one for this PR though.

@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Jan 14, 2020

Once this build is done we'll have to carefully check the doc build for any undesirable behavior.

Copy link
Member

jbednar left a comment

Looks fabulous! Reviewing the Reference Gallery at build.holoviews.org, it all looks correct to me for both Bokeh and Matplotlib except what might be pre-existing issues:

  • HexTiles has no padding; is that intentional? Tiles get cut off at the edges.
  • Violin gets cut off vertically for Bokeh, which might be more about an auto-ranging issue than a padding issue (as actual data gets cut off)presumably?)
@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Jan 15, 2020

Violin gets cut off vertically for Bokeh, which might be more about an auto-ranging issue than a padding issue (as actual data gets cut off)presumably?)

This is intentional and controlled by the cut option, without it outliers can make the plot useless.

@jbednar

This comment has been minimized.

Copy link
Member

jbednar commented Jan 15, 2020

This is intentional and controlled by the cut option, without it outliers can make the plot useless.

In that case is there a good reason why the Matplotlib and Bokeh Violin defaults are so different? The Matplotlib version doesn't cut off by default for the reference examples, but the Bokeh and to some extent Plotly versions do.

@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Jan 15, 2020

In that case is there a good reason why the Matplotlib and Bokeh Violin defaults are so different?

Yes, the matplotlib version uses the highly limited violin implementation that's shipped with matplotlib. Would be nice to replace that but it's a ton of work.

@philippjfr philippjfr force-pushed the enable_padding branch from e56d8d4 to fe5f5f5 Jan 15, 2020
@jbednar

This comment has been minimized.

Copy link
Member

jbednar commented Jan 15, 2020

I'll stop mentioning it now, but I do think the Matplotlib defaults are better for Violin, which I guess means I suggest using a higher cut threshold.

@philippjfr philippjfr merged commit e19ae88 into master Jan 15, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.