Skip to content

Add support for windowed auto-ranging#14353

Merged
bryevdv merged 23 commits intobranch-3.7from
bv/10020-windowed-ranging
Mar 6, 2025
Merged

Add support for windowed auto-ranging#14353
bryevdv merged 23 commits intobranch-3.7from
bv/10020-windowed-ranging

Conversation

@bryevdv
Copy link
Copy Markdown
Member

@bryevdv bryevdv commented Feb 22, 2025

Some example of the windowed auto-ranging in action are at the bottom of this OP.

Questions

  • Is a configuration like window_axis on Plot the best API? I thought about other options, e.g. a configuration on glyphs, or on data ranges. But ultimately, it can never make sense to have two ranges auto-range off each other at the same time. Therefore, my conclusion was that the configuration had to be at a "higher" level, i.e. on the Plot so that the possibility for that kind of mis-configuration could be avoided entirely.

  • Are there any thoughts or issues about the implementation approach itself? It turned out to be rather simple though possibly there are use cases I've overlooked. Currently verified that things work when window range is updated from stream following, or pan or zoom tools, also that reset tool turns windowed auto-ranging back on in case an interaction explicitly sets the auto-range extents. Also that having all the data "fall off" the window does not blow up anything.

  • Where is the best place to add documentation for this? cc @tcmetzger Are there other examples that could be updated besides these two?


ezgif-8b6b572bb05247
ezgif-8c0892f92980ab

@bryevdv bryevdv added this to the 3.7 milestone Feb 22, 2025
@bryevdv bryevdv force-pushed the bv/10020-windowed-ranging branch from 06525ab to 8c71b5f Compare February 22, 2025 02:03
@bryevdv bryevdv force-pushed the bv/10020-windowed-ranging branch from 8c71b5f to 302b858 Compare February 22, 2025 02:15
Comment thread bokehjs/test/integration/plots.ts Outdated
@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Feb 23, 2025

@mattpap I thought the CI would generate new baseline files for missing images, etc but when I download the new artifact there is nothing new in it to commit?

Edit also I thought we ran at least the unit tests in non-linux BokehJS CI jobs? If not and all we do is run the lint check, do we really actually need these extra jobs at all?

@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Feb 23, 2025

This is ready, modulo the question about how to get brand-new baseline files which seem to not be getting included in the report.

@mattpap
Copy link
Copy Markdown
Contributor

mattpap commented Feb 28, 2025

modulo the question about how to get brand-new baseline files which seem to not be getting included in the report.

This is Windows thing. This should work if you replace " with ' in baseline's name.

Comment thread src/bokeh/models/plots.py Outdated
Comment thread src/bokeh/models/plots.py Outdated
@mattpap
Copy link
Copy Markdown
Contributor

mattpap commented Feb 28, 2025

Release notes are missing. I will finish reviewing this tomorrow.

@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Mar 1, 2025

This is Windows thing. This should work if you replace " with ' in baseline's name.

@mattpap Sorry, I don't follow. I downloaded bokehjs-report.zip (which I think is generated on the ubunu job?) and unzipped it on my macbook, and this is all that's there:

base ❯ unzip bokehjs-report.zip
Archive:  bokehjs-report.zip
  inflating: bokehjs/test/baselines/linux/report.json
  inflating: bokehjs/test/baselines/linux/report.out

I am not using windows and AFAIK neither is the CI that generates the artifact. Can you be more specific about what needs updating, and where?

Edit: I guess maybe you mean change the "${wax}" in the test name to this?

      it(`with window_axis='${wax}' when range changes`, async () => {

maybe there is a way we can lint for this...

@mattpap
Copy link
Copy Markdown
Contributor

mattpap commented Mar 4, 2025

maybe there is a way we can lint for this...

I added validation in PR #14387.

Comment thread bokehjs/test/integration/plots.ts Outdated
Comment thread bokehjs/test/integration/plots.ts Outdated
Comment thread src/bokeh/models/plots.pyi
Comment thread bokehjs/src/lib/models/glyphs/glyph.ts Outdated
Comment thread bokehjs/src/lib/models/glyphs/glyph.ts Outdated
Comment thread bokehjs/test/integration/plots.ts Outdated
@bryevdv bryevdv requested a review from mattpap March 5, 2025 18:15
@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Mar 5, 2025

@mattpap this is ready for re-review

@mattpap
Copy link
Copy Markdown
Contributor

mattpap commented Mar 6, 2025

I really would like others on @bokeh/dev to join in help reviewing PRs. In particular in this PR at least discuss the API. Based on the discussion in the yesterday's meeting, Bokeh 3.8 should have a much shorter release cycle, about a month, and mostly include existing work. In particular any CSS breaking changes will be postponed for 3.9 (we will work in parallel to get CSS changes early in 3.9 development). Thus I would prefer to merge this PR in 3.8 and give us some time work with this and potentially refine this over the next month.

@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Mar 6, 2025

@mattpap do you have specific concerns? I did have some questions at first but that was a few weeks ago. I'm now personally quite happy with things at this point. I would very much like this to go in 3.7.

Edit: elaborating on this, I think the impossibility of supporting a "both" option places very hard constraints on the API. It has to go "above" the ranges themselves in order to coordinate, i.e. on the plot objects. So then it just comes down to naming, and I think this name is reasonable and lots of bike shedding is not needed. Beyond that, any implementation issues or bugs can be addressed in the next cycle.

Copy link
Copy Markdown
Contributor

@mattpap mattpap left a comment

Choose a reason for hiding this comment

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

I did have some questions at first but that was a few weeks ago. I'm now personally quite happy with things at this point. I would very much like this to go in 3.7.

I reviewed this on technical level and it's fine. However, I'm not sure if I'm the right person to approve this on API/functional level. If you think this is solid and you don't expect immediate changes in the next release, then I can approve it.

@bryevdv
Copy link
Copy Markdown
Member Author

bryevdv commented Mar 6, 2025

If you think this is solid and you don't expect immediate changes in the next release, then I can approve it.

👍 I don't expect any changes, fortunately the API surface needed is quite minimal.

@mattpap
Copy link
Copy Markdown
Contributor

mattpap commented Mar 6, 2025

OK, in this case PR is approved.

@bryevdv bryevdv merged commit 99212ff into branch-3.7 Mar 6, 2025
@bryevdv bryevdv deleted the bv/10020-windowed-ranging branch March 6, 2025 18:22
@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 Jun 18, 2025
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.

Windowed Auto-ranging

2 participants