-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Reimplement Path, Contours and Polygons plots #1991
Conversation
Yes, not quite done yet :-) |
Actually nevermind, it does work, I just hadn't declared County as a dimension. |
Additional user API is not part of this PR btw, the two main things I think we should still add are:
|
It looks like you've found a few more things to improve in this PR. I'll review it once you think it is ready again... |
It's ready, I just hadn't pushed stuff I'd done before. |
"green05 = levels.Overlay.Green[0.5]\n", | ||
"green05 + green05.Channel + green05.Channel.Green.sample(y=0.0)" | ||
"green05 = levels.Contours.Green\n", | ||
"green05 + chans.Channel.Green + chans.Channel.Green.sample(y=0.0)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this have implications for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess it has some, it's no longer overlaid
by default. I could have made it be overlaid again but I generally don't want to.
" for name, xs, ys, rate in zip(county_names, county_xs, county_ys, county_rates)}\n", | ||
"\n", | ||
"choropleth = hv.NdOverlay(county_polys, kdims=['County'])" | ||
"choropleth = hv.Polygons(counties, ['lons', 'lats'], [('detailed name', 'County'), 'Unemployment'])" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the old code still works and the new approach is just more succinct/flexible/efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
"Polygons with values can be used to build heatmaps with arbitrary shapes." | ||
"In order to efficiently represent the scalar values associated with each path the dictionary format is preferable since it can store the scalar values without expanding them into a whole column. Additionally it allows passing multiple columns as a single array by specifying the dimension names as a tuple.\n", | ||
"\n", | ||
"In this example we will createa list of random polygons each with an associated ``level`` value. Polygons will default to using the first value dimension as the ``color_index`` but for clarity we will define the ``color_index`` explicitly:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createa -> create a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@@ -155,7 +155,7 @@ | |||
"source": [ | |||
"%%opts Contours (linewidth=1.3) Image (cmap=\"gray\")\n", | |||
"cs = contours(penguins[:,:,'R'], levels=[0.10,0.80])\n", | |||
"cs" | |||
"penguins[:, :, 'R'] * cs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operation no longer just adds contours on top of the input and return it? That's fine as long as there is a switch to restore the old behavior. I also suspect we may have made this change a while back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you changed the default - see comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would at least need to document this change of behavior in the changelog/release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, may start requiring committers adding backwards compatibility changes to the CHANGELOG when they are made so we don't forget about them. Will do that here.
holoviews/core/data/interface.py
Outdated
gridded = False | ||
|
||
# Denotes whether the interface expects multiple ragged arrays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have to be arrays? Isn't it ragged 'data' (i.e multiple elements of different lengths)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
@@ -415,22 +415,17 @@ class contours(Operation): | |||
filled = param.Boolean(default=False, doc=""" | |||
Whether to generate filled contours""") | |||
|
|||
overlaid = param.Boolean(default=True, doc=""" | |||
overlaid = param.Boolean(default=False, doc=""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this answers my earlier question.
|
||
color_index = param.ClassSelector(default=None, class_=(util.basestring, int), | ||
allow_None=True, doc=""" | ||
Index of the dimension from which the color will the drawn""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to get the op
proposal in soon...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
tests/testoperation.py
Outdated
contour = Contours([[(-0.5, 0.416667, 0.5), (-0.25, 0.5, 0.5)], | ||
[(0.25, 0.5, 0.5), (0.5, 0.45, 0.5)]], | ||
vdims=img.vdims) | ||
print(contour.range(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray print.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I'll delete it.
@@ -20,7 +20,9 @@ class MultiInterface(Interface): | |||
|
|||
datatype = 'multitabular' | |||
|
|||
subtypes = ['dataframe', 'dictionary', 'array', 'dask'] | |||
subtypes = ['dictionary', 'dataframe', 'array', 'dask'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to try the more general dictionary (i.e standard python literals) format first. Might be other implications I haven't figured out yet. Then again, MultiInterface
is pretty new so it probably doesn't matter wrt backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the point here is that it doesn't accidentally expand your scalar values into a dataframe column for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Looks good. My main comments are:
If you haven't added material it would be good to see more examples (e.g of the split method). Anyway, none of this is critical and once a few of my comments above are addressed (typos, stray print and other minor things), I'm happy to merge. The main other thing I would like is more examples for the docs. |
Correct.
Yes,
This is a result of two things 1) it's no longer overlaid by default and b) contours now returns a single
Not sure what you're referring to, looks fine to me. Anyway, I completely overhauled the element reference notebooks adding more material.
I'm working on a "Working with Geometries" user guide which will cover stuff like this. Will see how far I get with that, but I can add it in a later PR. |
Ok great! Should contours get a compatibility flag in config? If we find ourselves with a number of small backwards incompatibility issues, we could group them under a new config flag. That doesn't need to be decided right away and I'll merge once you get rid of that stray print and fix that typo. |
The tests have now passed except for on transient error (build restarted and still running). Merging. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Over the past few months I've slowly been laying the groundwork to support more powerful Path, Contours and Polygons elements and in this PR I am also finally exposing this new functionality in plotting classes. This PR finishes off some final issues with the internally facing API and will reimplements the plotting classes based on this new API.
New features:
Here is a demo of the new (and old) features.
This PR will address the following issues:
and will allow me to finish off my geopandas interface in geoviews: