Skip to content

Set more sensible defaults in BoxAnnotation#13468

Merged
mattpap merged 1 commit into
branch-3.4from
mattpap/13432_box_annotation_defaults
Nov 28, 2023
Merged

Set more sensible defaults in BoxAnnotation#13468
mattpap merged 1 commit into
branch-3.4from
mattpap/13432_box_annotation_defaults

Conversation

@mattpap

@mattpap mattpap commented Oct 19, 2023

Copy link
Copy Markdown
Contributor

fixes #13432

@codecov

codecov Bot commented Oct 19, 2023

Copy link
Copy Markdown

Codecov Report

Merging #13468 (462d4d7) into branch-3.4 (8072a9a) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           branch-3.4   #13468   +/-   ##
===========================================
  Coverage       92.57%   92.57%           
===========================================
  Files             322      322           
  Lines           20460    20460           
===========================================
  Hits            18940    18940           
  Misses           1520     1520           

@mattpap mattpap force-pushed the mattpap/13432_box_annotation_defaults branch from 2826ff7 to a51238b Compare October 30, 2023 12:18
@mattpap mattpap force-pushed the mattpap/13432_box_annotation_defaults branch from a51238b to a777a7d Compare October 30, 2023 13:05
pink_line = p.line([1, 2, 3], [2, 1, 2], line_color="#CC79A7")

green_box = BoxAnnotation(left=1.5, right=2.5, fill_color='#009E73', fill_alpha=0.1)
green_box = BoxAnnotation(left=1.5, right=2.5, top=Node.frame.top, bottom=Node.frame.bottom, fill_color='#009E73', fill_alpha=0.1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this for demonstration, or required? I don't think having to explicitly specify this is an improvement, or what users would normally want to have to do to have the box be "unbounded" in a direction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's now required. Note that BoxAnnotation doesn't imply that it's bound to frame in any capacity (it's just a historical artifact), so unspecified positioning properties don't give a clue what the behavior will be (i.e. will it span, will it show at all?). It should be possible to attach it to anything like canvas or a panel or another box annotation. The syntax is indeed verbose, but at least the behavior is clear. BoxAnnotation wasn't ever a truly unbounded annotation, because it always draws lines on all sides and doesn't hit test at infinity, two things that {H,V}Strip glyphs do.

@bryevdv bryevdv Oct 31, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that BoxAnnotation doesn't imply that it's bound to frame in any capacity (it's just a historical artifact)

I'm not sure where this is coming from, but it is not correct. "missing bound = extend to plot edge" was an explicit design criteria of the original BoxAnnotation and is also what is and has been officially documented:

To define the bounds of these boxes, use the left/right or top/ bottom properties. If you provide only one bound (for example, a left value but no right value), the box will extend to the edge of the available plot area for the dimension you did not specify.

I'm pretty sure I am 👎 on this change as-is in general, but I am defintely 👎 on backporting such a breaking change to a point release.

@mattpap mattpap mentioned this pull request Oct 31, 2023
@mattpap mattpap force-pushed the mattpap/13432_box_annotation_defaults branch from a777a7d to 7855b49 Compare October 31, 2023 07:51
@bryevdv

bryevdv commented Nov 7, 2023

Copy link
Copy Markdown
Member

Attempting to regain some movement on this PR. @mattpap is the plan to deprecate BoxAnnotation entirely in favor of glyph alternatives? If so, then I would support these changes provided that the deprecation happens at the same time.

@mattpap mattpap force-pushed the mattpap/13432_box_annotation_defaults branch from 7855b49 to 462d4d7 Compare November 28, 2023 15:11
@mattpap

mattpap commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

I reverted the defaults, so that they are not required anymore and point to symbolic nodes of the frame.

@mattpap mattpap mentioned this pull request Nov 28, 2023
4 tasks
@mattpap mattpap merged commit a686cd4 into branch-3.4 Nov 28, 2023
@mattpap mattpap deleted the mattpap/13432_box_annotation_defaults branch November 28, 2023 16:47
@mattpap mattpap modified the milestones: 3.4, 3.3.2 Dec 24, 2023
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
@github-actions

Copy link
Copy Markdown

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 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] BoxAnnotation don't show up if not all lrbt-bounds are set

2 participants