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

Annotator overlays in ml_annotators #86

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jbednar
Copy link
Contributor

@jbednar jbednar commented Jan 15, 2020

Using the changes in holoviz/geoviews#429 and holoviz/holoviews#4184, it should now be possible to create an overlay and then pass that in to an annotator, as long as the overlay has only one annotable element. That capability makes it much easier to explain and illustrate how annotators work for overlaid data, because it lets us avoid explaining the confusing annotate.compose method until it is actually needed (in the case of multiple annotatable objects in the same overlay).

Assuming those PRs are completed and merged, this PR now introduces drawing tools on their own, using an overlay, and only then shows how that overlay can be used in an annotator. This approach helps introduce one concept at a time and let the reader keep everything straight.

To do:

  • The overlay support in Allow calling annotator on Overlay holoviz/holoviews#4184 is not yet ready; needs additional work before this notebook will run without errors and show the overlay.
  • Compatibility for changes in HoloViews holoviz/geoviews#429 and Allow calling annotator on Overlay holoviz/holoviews#4184 need to be merged
  • Need dev releases of panel, holoviews, and geoviews covering the above fixes
  • The changes to the .yml file should be validated against the actual value of the hv and gv dev releases needed; they were bumped by 1 but may not match where the actual changes end up
  • After the above changes are done, need to test this notebook to make sure that overlays show up everywhere and that the descriptions are accurate
  • How a user works with the PointDraw stream is different from how they work with the PointsAnnotator (e.g. annotator.annotated.dframe() vs pd.Dataframe(stream.data), no Shapely support that I can see, etc.). Are all of those differences intentional and unavoidable? If not it seems like it would be nice for people to be able to switch between the drawing-tool stream and the annotator as needed depending on whether they need an attached table or not, without seeing the APIs have to change entirely in the two cases. This is low priority, but worth considering.
  • The text probably could use a bit of updating once it all works, e.g. to explain streams a bit, to mention annotate.compose somewhere, and to mention that things get more complicated when overlaying multiple annotatable elements.

If it all works after the above, it should be merged and the running instance updated.

@jbednar jbednar added the wip Work in progress; do not merge label Jan 15, 2020
@maximlt
Copy link
Contributor

maximlt commented Jan 9, 2023

How a user works with the PointDraw stream is different from how they work with the PointsAnnotator (e.g. annotator.annotated.dframe() vs pd.Dataframe(stream.data), no Shapely support that I can see, etc.). Are all of those differences intentional and unavoidable? If not it seems like it would be nice for people to be able to switch between the drawing-tool stream and the annotator as needed depending on whether they need an attached table or not, without seeing the APIs have to change entirely in the two cases. This is low priority, but worth considering.

I'm not sure this is still valid, but if it is it would be better to open an issue where relevant as it can easily get lost here :)

Otherwise this looks like good improvements that could be merged in, after resolving the last TODO items and rebuilding the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress; do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants