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

Reimplement Path, Contours and Polygons plots #1991

Merged
merged 39 commits into from
Oct 22, 2017
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Oct 10, 2017

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:

  • Path, Contours and Polygons can now have any number of value dimensions associated with them which are revealed while hovering and which can be colored by.
  • Each geometry in a Contours or Polygons element can now have a different value associated with it, so you no longer have to create huge NdOverlays with one element per geometry.
  • Path now allows supplying varying value dimensions which are drawn as paths with continuously varying color.

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:

@jbednar
Copy link
Member

jbednar commented Oct 10, 2017

Fabulous! Was the county name meant to be visible in the hover?

image

@philippjfr
Copy link
Member Author

Fabulous! Was the county name meant to be visible in the hover?

Yes, not quite done yet :-)

@philippjfr
Copy link
Member Author

Actually nevermind, it does work, I just hadn't declared County as a dimension.

@philippjfr
Copy link
Member Author

@jlstevens This is ready for review once you have time.

@philippjfr
Copy link
Member Author

Additional user API is not part of this PR btw, the two main things I think we should still add are:

  • Allow groupby by value dimensions on Contours/Polygon (but not Path)
  • Allow selecting subpaths by index and value dimensions

@jlstevens
Copy link
Contributor

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...

@philippjfr
Copy link
Member Author

philippjfr commented Oct 20, 2017

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)"
Copy link
Contributor

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?

Copy link
Member Author

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'])"
Copy link
Contributor

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.

Copy link
Member Author

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:"
Copy link
Contributor

Choose a reason for hiding this comment

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

createa -> create a

Copy link
Member Author

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"
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

gridded = False

# Denotes whether the interface expects multiple ragged arrays
Copy link
Contributor

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)?

Copy link
Member Author

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="""
Copy link
Contributor

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""")
Copy link
Contributor

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

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))
Copy link
Contributor

@jlstevens jlstevens Oct 21, 2017

Choose a reason for hiding this comment

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

Stray print.

Copy link
Member Author

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']
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

@jlstevens
Copy link
Contributor

Looks good. My main comments are:

  • I am assuming old code still works and that many of the updates to the notebooks just show the new, cleaner way of doing things.
  • We need to be clear about any changes to behavior, e.g the way the contour function no longer overlays thing.
  • The change in how you use attribute access in the introduction notebook seems like the most worrying change I noticed...
  • Reading notebook diffs is tricky - I can't tell if you only updated the element notebooks or added material to showcase all the new functionality. Github's notebook rendering seems off (a problem with the HTML maybe?):

image

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.

@philippjfr
Copy link
Member Author

philippjfr commented Oct 22, 2017

I am assuming old code still works and that many of the updates to the notebooks just show the new, cleaner way of doing things.

Correct.

We need to be clear about any changes to behavior, e.g the way the contour function no longer overlays thing.

Yes, contours needs a note about backward compatibility.

The change in how you use attribute access in the introduction notebook seems like the most worrying change I noticed...

This is a result of two things 1) it's no longer overlaid by default and b) contours now returns a single Contours element not an NdOverlay of them so this falls under "noting contours backward compatibility changes".

Reading notebook diffs is tricky - I can't tell if you only updated the element notebooks or added material to showcase all the new functionality. Github's notebook rendering seems off (a problem with the HTML maybe?):

Not sure what you're referring to, looks fine to me. Anyway, I completely overhauled the element reference notebooks adding more material.

If you haven't added material it would be good to see more examples (e.g of the split method).

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.

@jlstevens
Copy link
Contributor

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.

@jlstevens
Copy link
Contributor

The tests have now passed except for on transient error (build restarted and still running).

Merging.

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

Successfully merging this pull request may close these issues.

None yet

3 participants