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

Fix data aspect issues #6189

Closed
wants to merge 2 commits into from
Closed

Fix data aspect issues #6189

wants to merge 2 commits into from

Conversation

ahuang11
Copy link
Collaborator

@ahuang11 ahuang11 commented Apr 12, 2024

The main issue here is that the current_l, current_r = plot.x_range.start, plot.x_range.end arrives late (or isn't up to date) so the data aspect calculation is messed up. There is, however, l, b, r, t = self.get_extents(element, ranges, dimension=range_dim) which is up to date.

Here, "current bounds" (inaccurate ylim) is 0.39 instead of 0.67 as seen on the plot axes. I could remove those lines, and the data aspect no longer bounces.

image

It was introduced in this PR #4569 and fixes holoviz/hvplot#462 so if I remove it, it reintroduces that error.

The only thing I'm pretty certain of is that if the plot isn't responsive, there's no need to re-calculate and ensure the data_aspect matches every time, which removes the bounciness:

Screen.Recording.2024-04-12.at.3.34.53.PM.mov

I think if there's a way to introduce value throttling, it could help.

Ultimately, I don't fully understand the difference between l, b, r, t = self.get_extents(element, ranges, dimension=range_dim) and plot.x_range & plot.y_range

@ahuang11 ahuang11 added the type: bug Something isn't correct or isn't working label Apr 12, 2024
@ahuang11 ahuang11 requested a review from philippjfr April 15, 2024 15:29
@philippjfr
Copy link
Member

@ahuang11 Please make sure you always paste the code for your reproducers.

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Apr 16, 2024

import panel as pn
import holoviews as hv
from holoviews.operation.datashader import rasterize
hv.extension("bokeh")

rasterize(hv.Curve([0, 1, 2])).opts(data_aspect=1)

@philippjfr
Copy link
Member

Am I meant to be able to reproduce an issue with that example? Because I can't.

@ahuang11
Copy link
Collaborator Author

Were you using this branch or main?

@philippjfr
Copy link
Member

main

@ahuang11
Copy link
Collaborator Author

To reproduce, I need to zoom a decent amount at once.

@ahuang11
Copy link
Collaborator Author

Screen.Recording.2024-04-16.at.7.26.36.AM.mov

@philippjfr
Copy link
Member

Okay, you should have specified that this is specific to VSCode (or rather to pn.config.comms = 'ipywidgets').

@hoxbro
Copy link
Member

hoxbro commented Apr 16, 2024

This could be related to the change I made in #5780

@philippjfr
Copy link
Member

Neither notebooks nor the server cases seem to be affected here.

@hoxbro
Copy link
Member

hoxbro commented Apr 16, 2024

For versions, you could run hv.show_versions() next time. This also outputs the comms.

@ahuang11
Copy link
Collaborator Author

Are you able to reproduce with this?
holoviz/hvplot#1105

VSCode (or rather to pn.config.comms = 'ipywidgets').

Okay I wasn't aware that VSCode notebook was different

@philippjfr
Copy link
Member

Are you able to reproduce with this?

Nope, no issues. Suspect there's some deeper issue syncing bokeh properties in the ipywidgets comms case.

@philippjfr
Copy link
Member

Okay I wasn't aware that VSCode notebook was different

The difference is that:

  • In classic notebook and JupyterLab we leverage regular notebook Comms, this allows for more direct communication
  • In VSCode and some other non-standard notebook environments comms are not available therefore we have to wrap our components in an IPyWidget wrapper (provided by jupyter-bokeh), this means we can shuttle the comms via the ipywidget comm.
  • On the server comms are handled via the dedicated websocket handler

In future just make sure you note which environment(s) you tested in.

@philippjfr
Copy link
Member

I suspect there's a deeper issue with syncing bokeh properties in the ipywidgets based scenario. Will keep digging.

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Apr 16, 2024

Okay, thanks for trying it out.

Anyhow, I think this PR might still be useful because I don't think the data aspect calculations (after the first) need to happen if the plot isn't responsive=True.

In future just make sure you note which environment(s) you tested in.

In the future, if I happen to forget, I default to VSCode

@philippjfr
Copy link
Member

Anyhow, I think this PR might still be useful because I don't think the data aspect calculations (after the first) need to happen if the plot isn't responsive=True.

This isn't true, axis labels can significantly impact the aspect.

@philippjfr
Copy link
Member

Really appreciate you chasing this btw. I just dug into the jupyter_bokeh IPyWidget wrapper and the issue seems to be to do with the serialization, it seems like the rangesupdate event that sends the updated ranges is not being serialized correctly which ends up serializing the entire plot (!!!) each time. This means the range update arrives late because there is such a big message being sent from frontend to the server.

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Apr 16, 2024

Edit: nevermind I see the internal plot's width being changed with the long tick axes.

axis labels can significantly impact the aspect.

I'm having a hard time wrapping my head around this. After initialization, the plot's frame_width + frame_height is set
so I don't think the axis labels will change that; I imagine the overflowed axis labels being cropped out like these tick labels

image
import numpy as np
import pandas as pd
import hvplot.pandas

df = pd.DataFrame(np.random.multivariate_normal((0,0), [[0.1, 0.1], [0.1, 1.0]], (500000,)))
df.hvplot.scatter("0", "1").opts(xlabel="abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef")
image

@philippjfr
Copy link
Member

I'm having a hard time wrapping my head around this

Me too, me too!

After initialization, the plot's frame_width + frame_height is set

Will have to very carefully review that bit of code to make sure that always holds true.

Also my apologies, I meant the tick labels, not the axis labels.

@philippjfr
Copy link
Member

Turns out this was caused by a big oversight in jupyter_bokeh fixed here: bokeh/jupyter_bokeh#204

The problem was that random events could end up being dropped and this reliably affected the end of the x_range, so the aspect calculation was thrown completely off by the fact that the x_range.end was incorrect.

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Apr 18, 2024

I'm wondering if there's an issue in Jupyter notebook too because in Jason's Gerrymandering recording, we see data aspect issue at around 5s.

holoviz-topics/examples#369 (comment)

Gerrymanding.Final.Part.mp4

@ahuang11 ahuang11 closed this Apr 23, 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.

hvplot.image() zoom causes axes limits to bounce around
3 participants