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

Replace data annotations with glyphs #13344

Open
wants to merge 21 commits into
base: branch-3.5
Choose a base branch
from

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Aug 22, 2023

This is a proof of concept and a breaking change. This can be easily made into a non-breaking change by either naming the glyph differently or introducing two glyphs ({H,V}Whisker), similarly to what was done with Span annotation vs. span-like glyphs (PR #12677). The functionality is preserved except for screen units. Those can or will be possible to reproduce with sub-coordinates (a part of my current CZI work), though I'm not sure about usefulness of that. I also added figure().whisker() for API completeness.

image

addresses #9955

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Attention: Patch coverage is 93.12500% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 92.63%. Comparing base (e61219b) to head (92fe31a).
Report is 40 commits behind head on branch-3.5.

❗ Current head 92fe31a differs from pull request most recent head 7b8ca2c. Consider uploading reports for the commit 7b8ca2c to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##           branch-3.5   #13344      +/-   ##
==============================================
+ Coverage       91.57%   92.63%   +1.05%     
==============================================
  Files             326      327       +1     
  Lines           20737    20787      +50     
==============================================
+ Hits            18990    19255     +265     
+ Misses           1747     1532     -215     

@bryevdv
Copy link
Member

bryevdv commented Aug 22, 2023

I am 100% behind the migrating towards a glyph, but I really don't like "VWhisker" and "HWhisker". I'll have to think some on what the least disruptive path for users might be (or less awkward "V" and "H" names).

@bryevdv
Copy link
Member

bryevdv commented Aug 31, 2023

Noting that Band is another good candidate to split and make a glyph, considering the confusion here:

https://discourse.bokeh.org/t/rotating-a-figure/10802

In that case vband and hband seem fine, maybe for consistency we just have to hold our noses and live with "hwhisker" cc @tcmetzger in case you have any thoughts

@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch 3 times, most recently from 406c044 to cb01d0a Compare September 20, 2023 20:12
@mattpap mattpap modified the milestones: 3.x, 3.4 Sep 20, 2023
@mattpap
Copy link
Contributor Author

mattpap commented Sep 21, 2023

I was able to provide a legacy implementation for Band and Whisker annotations, which retains most of the original functionality (e.g. no auto-ranging), with the exception of screen coordinates (which will be possible with some improvements to sub-coordinates). I was able to do that, by keeping bokeh.models.{Band,Whisker} referring to annotations and not glyphs, and by not exporting Band and Whisker glyphs from bokeh.models.glyphs. As a side effect of this, I have some observations and a proposal to make.

We shouldn't make API design decisions (in this case whether there should be one or two band and whisker glyphs) based on our self imposed restriction of having a flat bokeh.models namespace for all models. I think the project outgrew that idea and there should be one level of nesting to avoid situations like the one in this PR or elsewhere (e.g. in PR #13286, where I have to add namespacing suffixes or prefixes to avoid name collisions). I believe this is fine, because as far I'm concerned, I always thought models to be low-level entities, that normally would be constructed through higher level APIs, like we have for glyphs, layouts and to some extent expressions. In particular I think it's a huge waste to both have high level glyphs functions and expose all glyph models via bokeh.models.

So my preliminary proposal is stop exposing new models via bokeh.models namespace and seal currently exposed models in bokeh/models/__init__.py, while still allowing __all__ for sub-modules (e.g. bokeh.models.glyphs) to contain all public models from that sub-module. Introduce (preferably namespaced) high level APIs for annotations, etc. (e.g. figure.annotation.box()). This could be applied to existing marker methods, i.e. instead of deprecating and removing them at some point, we could deprecate them and move them to a figure.marker namespace, and then remove the original at some point. Changes will be needed at the protocol level, which is easy, and some effort will be required to make that backwards compatible. Finally at bokeh 4.0 we would remove exports from bokeh/models/__init__.py.

@bryevdv
Copy link
Member

bryevdv commented Sep 21, 2023

I thought we had alignment on the POV that "annotation is something a user does, not a particular kind of object" i.e. "making everything a glyph". So then having e.g. figure.annotation.box() would really go against that in mind. This will obviously require a thorough discussion with more people, so I'd suggest opening a detailed GH discussion around it first of all.

Regarding this:

In particular I think it's a huge waste to both have high level glyphs functions and expose all glyph models via bokeh.models.

The models exist for the implementation and our use as maintainers. The functions exist because users really hate having to import 30 things and want to do more with less code.. Having both really does not bother me at all, because they serve entirely different audiences with different purposes.

@mattpap mattpap changed the base branch from branch-3.3 to branch-3.4 October 10, 2023 12:45
@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch 2 times, most recently from f8557dc to 108a14c Compare October 11, 2023 11:24
@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch 2 times, most recently from 2b44ca0 to ceda2a7 Compare December 6, 2023 20:44
@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch 2 times, most recently from 9126d7f to 9e44da0 Compare January 11, 2024 00:02
@mattpap mattpap modified the milestones: 3.4, 3.5 Jan 23, 2024
@mattpap mattpap changed the base branch from branch-3.4 to branch-3.5 March 14, 2024 16:52
@mattpap
Copy link
Contributor Author

mattpap commented Apr 4, 2024

TODO:

  • finish implementation of HTMLText glyph
  • add support for "screen" coordinate units (via coordinate transforms)

@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch from 9a4e192 to 92fe31a Compare April 4, 2024 09:45
@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch from 92fe31a to 9d7d1ae Compare May 8, 2024 07:05
@bryevdv
Copy link
Member

bryevdv commented May 8, 2024

@mattpap you mentioned needed to add screen units support to annotation glyphs as pert of finishing this. Does that extend to all glyphs? A user on the discourse was asking about screen positioning for wedges, and I was not sure if that was planned. https://discourse.bokeh.org/t/annular-wedge-position-in-screen-units/11526

@mattpap
Copy link
Contributor Author

mattpap commented May 8, 2024

Does that extend to all glyphs?

This will be implemented via a coordinate transform (what sub-coordinates use), so it will be available to all glyphs and in fact to all renderers.

@mattpap
Copy link
Contributor Author

mattpap commented May 13, 2024

Preliminary support coordinate systems that don't involve the frame (subcoordinates) has landed:

Screencast.from.13.05.2024.17.35.54.webm

One missing bit is symbolic ranges to make to fully operational and to allow implementing screen coordinates in legacy APIs.

@mattpap mattpap force-pushed the mattpap/9955_whisker_glyph branch from ae96e8c to 7b8ca2c Compare May 15, 2024 08:24
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

2 participants