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

Add ppi argument for saving and displaying charts as PNG images #3163

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Aug 25, 2023

This PR adds support for a new ppi argument to control the pixels-per-inch metadata of the PNG files generated by vl-convert 0.13.0.

For background discussion on ppi see:

Saving PNG files with ppi

import altair as alt
from vega_datasets import data

source = data.cars()

chart = alt.Chart(source).mark_boxplot(extent="min-max").encode(
    alt.X("Miles_per_Gallon:Q").scale(zero=False),
    alt.Y("Origin:N"),
)
chart.save("box.png", scale=2, ppi=300)

This will produce a file named box.png that is scaled to be twice as large as the original chart, with a ppi of 300. Not all image viewers respect PPI when displaying PNG images, but Preview on MacOS at least shows the ppi metadata in the info panel:

Screenshot 2023-08-25 at 10 11 35 AM

Display as PNG

Chart's can be displayed as PNG images with

alt.renderers.enable("png", scale_factor=2, ppi=200)

LaTeX

When exporting notebooks to PDFs with LaTeX the ppi metadata is used to determine the physical size that the image should be displayed at.

Jupyter

Jupyter / Browsers don't seem to use the png image's ppi metadata to determine the size of the output image. To get this behavior, we can follow the logic that the IPython.display.Image uses when the retina argument is set to True. I followed the implementation in https://github.com/ipython/ipython/blob/main/IPython/core/display.py#L809 to learn how to control the displayed width/height of a PNG mime bundle in Jupyter.

This involved swiping the _pngxy function to extract the image width/height from the png image bytes returned by vl-convert.

Here's what this looks like in JupyterLab:

Screenshot 2023-08-25 at 10 20 33 AM

@jonmmease
Copy link
Contributor Author

@joelostblom Could you try LaTeX to PDF output with this branch, vl-convert-python 0.13.0, and with image rendering enabled with alt.renderers.enable("png", ppi={v})? I want to make sure that the extra metadata to control the width/heigh of the image in Jupyter doesn't throw off the LaTeX calculations.

Also, if you have a chance, would you mind taking pass at explaining ppi somewhere in the docs? It doesn't really show up in the docstrings of alt.renderers.enable or chart.save, so it would be good to make it discoverable in the docs.

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

The Latex PDF output seems to work! Thank you!

image

Note that when I run it with an older version of altair, it runs without an error but produces the wrong output (only scales but does not adjust the ppi). I wonder if this can be confusing, making it seem like the ppi option is not working. Do you think we could add a check for if the ppi parameter is used with an incompatible version of vl-convert/altair and raise a warning/error?

image

Also note the warning in the topmost screenshot regarding anywidget. Should we silence that since it might be confusing for users not knowning that they are using naywidget with latiar?

@jonmmease
Copy link
Contributor Author

Also note the warning in the topmost screenshot regarding anywidget. Should we silence that since it might be confusing for users not knowning that they are using naywidget with latiar?

@manzt Is there something we should be doing to avoid the "Live reloading is disabled..." warning when not in development mode?

@jonmmease
Copy link
Contributor Author

Thanks for trying it out @joelostblom, glad it's working! Regarding compatibility. Altair 5.1 will require vl-convert 0.13.0 which has ppi support.

I don't think there's anything we can do retroactively to warn users of Altair 5.0.1 that ppi isn't supported unfortunately. Does that make sense?

)
return {"image/png": png}
factor = ppi / 72
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment why ppi is devided by 72?

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Looks good to me, also tested it locally. Only one minor request in the code and also could you add a line to the changelog? Thank you @jonmmease! This is very handy.

@binste
Copy link
Contributor

binste commented Aug 25, 2023

Also note the warning in the topmost screenshot regarding anywidget. Should we silence that since it might be confusing for users not knowning that they are using naywidget with latiar?

@manzt Is there something we should be doing to avoid the "Live reloading is disabled..." warning when not in development mode?

+1 for silencing the warning

@manzt
Copy link
Contributor

manzt commented Aug 25, 2023

@manzt Is there something we should be doing to avoid the "Live reloading is disabled..." warning when not in development mode?

Hi all, thanks for pinging me. The "watch mode" is automatically enabled if the widget file is outside of "site-packages" (e.g., a development install).

https://github.com/manzt/anywidget/blob/fdb61c3c1e4be31998e532b1948e2ed07addb19e/anywidget/_util.py#L167-L185

There currently isn't a way to disable this behavior (i.e. starting a watch thread for a file outside of site-packages), but you can suppress the message by having watchfiles installed in your environment. I'm more than happy to consider any suggestions to improve the developer experience. Maybe just an environment variable you could configure in a Jupyter environment, although I do like the automatic behavior for most use cases...

import os

os.environ["ANYWIDGET_WATCH"] = True

class MyWidget(anywidget.AnyWidget):
    ...

@jonmmease
Copy link
Contributor Author

Thanks @manzt, the site-packages convention makes sense. I'll add watchfiles to our development environment and then double check that we don't see the warning when installing into site-packages.

@jonmmease
Copy link
Contributor Author

@joelostblom and @binste, I added a changes entry and updated the save chart documentation to discuss ppi and scale_factor. We don't really have much documentation on the individual renderer types, so I didn't see a clear place to document the ppi argument to alt.renderer.enable for now.

@jonmmease jonmmease merged commit 45cd440 into main Aug 25, 2023
20 checks passed
@binste
Copy link
Contributor

binste commented Aug 25, 2023

Thanks @manzt, the site-packages convention makes sense. I'll add watchfiles to our development environment and then double check that we don't see the warning when installing into site-packages.

Agree. Thanks @manzt for the fast response! I was worried that users would see this but this makes sense.

@joelostblom and @binste, I added a changes entry and updated the save chart documentation to discuss ppi and scale_factor. We don't really have much documentation on the individual renderer types, so I didn't see a clear place to document the ppi argument to alt.renderer.enable for now.

Looks good, thanks Jon!

@mattijn
Copy link
Contributor

mattijn commented Aug 25, 2023

I get the following error when running the altair specification.

File C:\Notebooks\altair\altair\altair\utils\mimebundle.py:136, in _spec_to_mimebundle_with_engine(spec, format, mode, **kwargs)
    130     png = vlc.vega_to_png(
    131         spec,
    132         scale=scale,
    133         ppi=ppi,
    134     )
    135 else:
--> 136     png = vlc.vegalite_to_png(
    137         spec,
    138         vl_version=vl_version,
    139         scale=scale,
    140         ppi=ppi,
    141     )
    142 factor = ppi / default_ppi
    143 w, h = _pngxy(png)

TypeError: vegalite_to_png() got an unexpected keyword argument 'ppi'

Is this possible? I assumed import_vl_convert() should raise an ImportError?

@jonmmease
Copy link
Contributor Author

Oh, good catch! We're not using import_vl_convert here yet. I'll update that shortly

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.

5 participants