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

Enhanced Annotators and clarified docs #4167

Merged
merged 8 commits into from
Jan 8, 2020
Merged

Enhanced Annotators and clarified docs #4167

merged 8 commits into from
Jan 8, 2020

Conversation

jbednar
Copy link
Member

@jbednar jbednar commented Jan 8, 2020

Some changes worth noting/explaining:

  • I found it very confusing to introduce the annotator without .instance() first, then have to say it's unusable. Given that doing so is a very strange thing, I opted to use .instance right away, even if it's mysterious.
  • To emphasize that annotate is the underlying call and annotator is the thing you'll actually use, I removed the import of annotate and systematically referred to it using hv.annotate instead. I think that helps keep the two things straight; one is an object you can work with in your namespace (annotator); the one provided by the HoloViews library is just for getting something you actually then use, not something to call or work with directly. That's the hope, anyway!
  • Changed integer coordinates to floats where it was necessary to avoid issue Confusing behavior for integer Annotator initial values #4166.
  • Added Scatter to the list of supported elements (but there's no Scatter example; should say in words how to make one).
  • Commented-out the last cell, which is very confusing on a statically rendered notebook, as the output no longer matches the cell code when reading through it. Should now be ok.

Ready to merge, but there are things left to do (presumably in a later PR, as this one should be merged to make the docs be usable now):

  • Should explain better what to do for types not supported yet (cast to supported type, write new hv code, add Bokeh tools, ?)
  • Annotating paths/polygons needs much better docs; it's really hard to figure out how to use it right now. I didn't try to update that section, but not because it doesn't need it!

@jbednar jbednar requested a review from philippjfr January 8, 2020 04:27
@philippjfr
Copy link
Member

All sounds sensible.

I found it very confusing to introduce the annotator without .instance() first, then have to say it's unusable. Given that doing so is a very strange thing, I opted to use .instance right away, even if it's mysterious.

That was done in response to a review comment on the original PR, I do agree.

Annotating paths/polygons needs much better docs; it's really hard to figure out how to use it right now. I didn't try to update that section, but not because it doesn't need it!

I didn't want to repeat the explanations from the PolyDraw/PolyEdit stream reference docs but maybe I should.

@philippjfr
Copy link
Member

I've pushed some additional fixes/changes to this PR:

  • Renamed BoxAnnotator->RectangleAnnotator
  • Selecting a Path/Polygon in the first table now updates the vertex table
  • Exposed the empty_value as a parameter on Annotators
  • Expanded docs on using the PolyDraw/PolyEdit tools in the Annotator docs

@philippjfr
Copy link
Member

Lastly I added a CurveEdit stream and CurveAnnotator.

@philippjfr philippjfr merged commit ff26fed into master Jan 8, 2020
@jbednar jbednar deleted the annodocs branch January 8, 2020 18:43
@jlstevens jlstevens changed the title Expanded and clarified hv annotator docs Enhanced Annotators and clarified docs Mar 20, 2020
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants