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

Apply autorange on data change events #5609

Merged
merged 6 commits into from
Feb 6, 2023
Merged

Apply autorange on data change events #5609

merged 6 commits into from
Feb 6, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 3, 2023

  • Had to schedule the CustomJS callback using setTimeout because the ColumnDataSource.data callback fires before the glyphs (and more importantly their indexes) are updated.
  • We remove all usage of cb_obj in the callback which allows us to reuse the callback between both data and range updaates.
  • We add the autorange callback to a new _js_on_data_callbacks instance variable which then gets applied to all sources attached to a GlyphRenderer

ToDo

  • Check inverted axes work with this
  • Guard against failed lookups in Bokeh.index
  • Guard the view.child_views lookup to avoid errors in layouts
  • Somehow filter out GlyphRenderers which were generated by plots with apply_ranges=False (suggest using .tags property to mark them)

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #5609 (7f0a876) into main (a441bfa) will decrease coverage by 0.03%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##             main    #5609      +/-   ##
==========================================
- Coverage   88.29%   88.27%   -0.03%     
==========================================
  Files         302      302              
  Lines       62460    62495      +35     
==========================================
+ Hits        55152    55168      +16     
- Misses       7308     7327      +19     
Impacted Files Coverage Δ
holoviews/plotting/bokeh/element.py 88.17% <44.44%> (-0.44%) ⬇️
holoviews/core/accessors.py 83.72% <0.00%> (-1.66%) ⬇️
...loviews/tests/plotting/matplotlib/test_renderer.py 97.50% <0.00%> (-1.21%) ⬇️
holoviews/operation/datashader.py 83.90% <0.00%> (-0.09%) ⬇️
holoviews/tests/core/test_dynamic.py 99.45% <0.00%> (+<0.01%) ⬆️
holoviews/tests/plotting/bokeh/test_callbacks.py 98.65% <0.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jlstevens
Copy link
Contributor

I think holoviews is doing what I asked for here even though the behavior is a little crazy!

flipping_inverted

@jbednar
Copy link
Member

jbednar commented Feb 3, 2023

I think holoviews is doing what I asked for here even though the behavior is a little crazy!

That doesn't seem right at all. inverting should be about how that particular plot is displayed, not any other plot...

@philippjfr
Copy link
Member Author

That doesn't seem right at all. inverting should be about how that particular plot is displayed, not any other plot...

Linked axes are shared axes, so there's no way to avoid this.

@jbednar
Copy link
Member

jbednar commented Feb 3, 2023

Linked axes are shared axes, so there's no way to avoid this.

I'd say that there's no current way. :-) I'd consider this a bug, since I think the intent of the user is clear here: share the range extents, don't share the orientation. Addressing it seems very difficult, so maybe there can be a warning if axes have different values for inversion, saying that invert_{xy}axis must be set to the same value on all linked axes (so that users realize they need to set it on both or neither, with the current implementation).

@jlstevens
Copy link
Contributor

I think a warning for now and an issue to record the eventual desired behavior is the way forward for now...

@jlstevens
Copy link
Contributor

@philippjfr I've finished off the tasks mentioned in your TODO list - the tags suggestion works great! Ready for review/merge once the test go green.

Copy link
Member Author

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

Happy to see this merged. Since this is my PR I can't approve, so feel free to approve yourself and merge.

@jlstevens
Copy link
Contributor

I'll add a line to the user guide about auto ranging applying after data updates and then I'll merge.

@jlstevens
Copy link
Contributor

I'll go ahead and merge.

@jlstevens jlstevens merged commit c8744d6 into main Feb 6, 2023
@jlstevens jlstevens deleted the autorange_improve branch February 6, 2023 14:53
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants