-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Allow to zoom secondary ranges/scales independently #13049
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
Conversation
What are you envisioning for the API / properties (independent vs synced will need to be configurable somehow)? |
Codecov Report
@@ Coverage Diff @@
## branch-3.2 #13049 +/- ##
===========================================
Coverage 92.43% 92.43%
===========================================
Files 315 315
Lines 20072 20078 +6
===========================================
+ Hits 18554 18560 +6
Misses 1518 1518 |
8b28b7b
to
9029e2d
Compare
9029e2d
to
05cbfa6
Compare
52b566b
to
ddfc933
Compare
ddfc933
to
4f1e2bb
Compare
Thanks! I'll test out and review the functionality shortly. |
I've done some initial tests and it seems to be working great! @mattpap My main worry is that I really would love to see this is 3.2! |
4f1e2bb
to
d6a4924
Compare
Unit tests are update. I still need to finish new tests. |
I've only had a quick look and I can review properly tomorrow. But my first observation is that if I use |
If we keep this then I think E.g. just spitballing, |
I don't have any strong opinions on this but I think a new property might indeed be slightly cleaner as there are two behaviors here: 1. whether to have per-axis zooming at all 2. how this links to other axes when enabled. Even if a new property is not introduced, I do agree that As the word 'linked' has already been used (and is somewhat intuitive to me), how about a property called |
@jlstevens, would you prefer to test this in holoviews as-is or wait for this PR to be refined? |
@mattpap I assume the tweaks suggested to the API won't change the behavior at all: I'm happy to test with whichever API in holoviews and the sooner the better! If my assumption is wrong and you think the available set of behavior will change, then in that case, I think it would be best to wait. |
Screencast_00000.mp4
Screencast_00001.mp4
Screencast_00002.mp4 |
d6a4924
to
e09eb3e
Compare
I updated the API, so that |
I will need some someone to approve this if the plan is sound. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the API, so that
zoom_on_axis
is left unchanged and new propertyzoom_together
is added. I chose"cross"
in favor of"frame"
. My proposal is to go ahead and merge this, to allow @jlstevens test this in holoviews. We can further refine the API before 3.2. I will separately add more tests for new features.
Thanks for the videos and expanded example, it is all much clearer to me. Functionality and code look fine to me, so let's proceed and, as you say, refine the API before 3.2.
* Allow to zoom secondary ranges/scales independently * Add support for ZoomBaseTool.renderers * Update basic/axes/twin_axes.py * Allow to configure individual zoom_on_axis behavior * Robustify and update unit tests * Update WheelZoomTool's off-frame unit tests * Unify WheelZoomTool's unit tests * Refine the API of on-axis zooming * Update twin_axes.py example
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. |
This is functionally ready. This PR allows for wheel zoom of individual axes (
zoom_on_axis="individual"
) and on linked (or perhaps a better term is cross) axes (zoom_on_axis="individual_linked"
). The original behavior ofzoom_on_axis
with a boolean value is left unchanged. AdditionallyZoomInTool
andZoomOutTool
can be configured withrenderers
, which are used to determine which ranges will be affected by zoom:This example shows
WheelZoomTool(zoom_on_axis="individual")
, andZoomInTool
andZoomOutTool
configured with a single renderer:Screencast_00001.mp4
TODO:
fixes #12829