Handle new FigureCanvasBase.device_pixel_ratio. by QuLogic · Pull Request #317 · matplotlib/ipympl · GitHub
Skip to content

Handle new FigureCanvasBase.device_pixel_ratio. - #317

Merged
ianhi merged 1 commit into
matplotlib:masterfrom
QuLogic:dpi-update
Apr 16, 2021
Merged

ianhi merged 1 commit into
matplotlib:masterfrom
QuLogic:dpi-update

Conversation

@QuLogic

@QuLogic QuLogic commented Apr 6, 2021

Copy link
Copy Markdown
Member

This is a corollary to
matplotlib/matplotlib#19126.

@github-actions

github-actions Bot commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

@QuLogic

QuLogic commented Apr 6, 2021

Copy link
Copy Markdown
Member Author

There's a _dpi_ratio thing in Canvas, but I'm not sure if I need to do the same for _device_pixel_ratio (as set in FigureCanvasBase instead of FigureCanvasWebAgg).

@ianhi

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator

Thanks!

This looks safe to merge prior to the the matplotlib PR correct? I think it makes sense to merge this prior to #314

@QuLogic

QuLogic commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

It should be safe yes, as the new message will just be logged as unknown in older Matplotlib.

@ianhi

ianhi commented Apr 8, 2021

Copy link
Copy Markdown
Collaborator

There's a _dpi_ratio thing in Canvas, but I'm not sure if I need to do the same for _device_pixel_ratio (as set in FigureCanvasBase instead of FigureCanvasWebAgg).

That's all the way back from the very first commit 047ed95 with a comment by saying: # Must declare the superclass private members. @SylvainCorlay you left that comment do you know why they need to be declared? It's not clear to me why we need to transform those into traitlet variables if we aren't syncing them to the frontend.

@ianhi

ianhi commented Apr 15, 2021

Copy link
Copy Markdown
Collaborator

Actually looking back at #264 I'm not sure that that comment is still relevant. At the very least nothing bad happened when we removed all those traitlets.

So in conclusion I don't think you need to change anything more. If anything we could even remove the _dpi_ratio being a traitlet.

@ianhi
ianhi merged commit beec42b into matplotlib:master Apr 16, 2021
@ianhi

ianhi commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

Thanks!

@QuLogic
QuLogic deleted the dpi-update branch April 16, 2021 21:36
@SylvainCorlay

Copy link
Copy Markdown
Member

Sorry I should have looked into this earlier.

One issue with fetching the device pixel ratio from the front-end (which is what we used to do in the past) is that it may depend on the browser window, so that different views of the same figure would have different values for that property.

@ianhi

ianhi commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

One issue with fetching the device pixel ratio from the front-end (which is what we used to do in the past)

As far as I can tell that was how ipympl behaved prior to this PR as well. This PR just sends the same information to where it needs to be on the python side for mpl 3.5.

Is there any way to deal with this? It seems to me that the frontend GUI framework is necessarily the source of truth for this information.

@SylvainCorlay

Copy link
Copy Markdown
Member

As far as I can tell that was how ipympl behaved prior to this PR as well. This PR just sends the same information to where it needs to be on the python side for mpl 3.5.

I thought I removed that logic at some point because of that problem. If I recall correctly, we decided for an arbitrary fixed number (which could be overwritten in the backend).

Is there any way to deal with this? It seems to me that the frontend GUI framework is necessarily the source of truth for this information.

Setting an arbitrary number may be good enough, since the ipympl front-end isn't really meant for print, pixel-units may be good enough.

@ianhi

ianhi commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

I thought I removed that logic at some point because of that problem. If I recall correctly, we decided for an arbitrary fixed number (which could be overwritten in the backend).

I think this is quantity we are talking about?

this.ratio = (window.devicePixelRatio || 1) / backingStore;

On my monitor at least (not hidpi) this is mostly affected by the zoom level of the jupyterlab tab.

@SylvainCorlay

Copy link
Copy Markdown
Member

What I meant is that it is impacted if you open the same notebook with two different computers at the same time. As we are working on live collaboration, this may happen in the near future.

@SylvainCorlay

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants