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

Ensure Range streams and RangeToolLink respect subcoordinate axis range #6256

Merged
merged 14 commits into from
Jun 7, 2024

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 4, 2024

This PR handles the previously poorly defined behavior of Range streams and RangeToolLink attached to a subcoordinate axis. Specifically both now pick up the range of the outer subcoordinate axis range rather than one of the inner subcoordinate ranges. The latter case is still something that will be needed in certain cases so that is still a problem to be solved.

Fixes #6010 and #6136

@philippjfr philippjfr added the type: bug Something isn't correct or isn't working label Jun 4, 2024
@philippjfr
Copy link
Member Author

@droumis please test when you get a chance.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 89.36170% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.52%. Comparing base (9cc87fe) to head (683c604).
Report is 8 commits behind head on main.

Files Patch % Lines
holoviews/tests/ui/bokeh/test_callback.py 55.55% 8 Missing ⚠️
holoviews/plotting/bokeh/links.py 93.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6256      +/-   ##
==========================================
+ Coverage   88.47%   88.52%   +0.04%     
==========================================
  Files         323      323              
  Lines       67647    67894     +247     
==========================================
+ Hits        59853    60103     +250     
+ Misses       7794     7791       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahuang11
Copy link
Collaborator

ahuang11 commented Jun 4, 2024

It seems to work. Awesome!
image

image

@ahuang11
Copy link
Collaborator

ahuang11 commented Jun 4, 2024

Actually the ranges look right, but when I zoom in and slice, I get a blank screen.

image

@droumis
Copy link
Member

droumis commented Jun 5, 2024

Looks good! Tested a few different versions, including multiple channel groups, with downsample1d, with wide df common index slice optimization, and with the large data pyramid approach.

Screen.Recording.2024-06-05.at.10.56.00.AM.mov
Screen.Recording.2024-06-05.at.11.01.52.AM.mov

@philippjfr
Copy link
Member Author

@ahuang11 Remind me what the issue we encountered in the CZI call yesterday was?

@ahuang11
Copy link
Collaborator

ahuang11 commented Jun 5, 2024

After zooming in, then zooming out, the curves end up offset from the yticks.

@philippjfr
Copy link
Member Author

Ah, yes, thanks!

@philippjfr
Copy link
Member Author

@droumis @ahuang11 Should be fixed now.

@ahuang11
Copy link
Collaborator

ahuang11 commented Jun 5, 2024

Works for me!

@hoxbro hoxbro added this to the 1.19.0 milestone Jun 6, 2024
@philippjfr
Copy link
Member Author

@hoxbro This should be ready to review again.

Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

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

I haven't tested the code. I hope either @droumis or @ahuang11 can do that.

Some small comments, mostly nit.

Also needs to update TestCurvePlot.test_batched_curve_subscribers_correctly_linked

holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/tests/plotting/bokeh/test_callbacks.py Outdated Show resolved Hide resolved
holoviews/tests/plotting/bokeh/test_callbacks.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/links.py Outdated Show resolved Hide resolved
@droumis
Copy link
Member

droumis commented Jun 6, 2024

This is amazing (combined with the hold render changes in #6265. Both for notebook and standalone.

video1254322944.mp4

@philippjfr philippjfr requested a review from hoxbro June 7, 2024 08:31
holoviews/plotting/plot.py Show resolved Hide resolved
holoviews/plotting/bokeh/links.py Outdated Show resolved Hide resolved
@hoxbro hoxbro enabled auto-merge (squash) June 7, 2024 11:00
@hoxbro hoxbro merged commit 4db648d into main Jun 7, 2024
14 checks passed
@hoxbro hoxbro deleted the subcoord_y_range_stream_and_link branch June 7, 2024 11:21
@hoxbro hoxbro mentioned this pull request Jun 10, 2024
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 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlaying annotations on target plot breaks RangeToolLink
4 participants