has2k1 / plotnine

A Grammar of Graphics for Python
https://plotnine.org
MIT License
3.89k stars 209 forks source link

Specifying dpi option in theme does not work in quarto #773

Closed machow closed 2 months ago

machow commented 2 months ago

When using quarto with a qmd, setting the theme(dpi=...) option does not seem to affect plot output.

example.qmd

---
format:
    html:
       # setting fig-dpi does affect DPI
       # fig-dpi: 400
       fig-dpi: 100
---

```{python}
from plotnine import *
import pandas as pd

df = pd.DataFrame({"x": [1,2,3], "y": [4,5,6]})
ggplot(df, aes("x", "y")) + geom_point()
ggplot(df, aes("x", "y")) + geom_point() + theme(figure_size=(6, 4), dpi = 400)


Both plots produced for me are around 8kb, and about 600 x 400px. Note that leaving out the `fig-dpi` option completely in the top-matter results in a DPI setting of 100 (or similar) in the rcParams.

### Possible causes

Pieces I noticed digging around:

* quarto sets [a number of matplotlib settings](https://github.com/quarto-dev/quarto-cli/blob/1ec01151ad59930b6042abed34ee9f90fd1826ab/src/resources/jupyter/lang/python/setup.py#L19-L26) before running python qmds
* uncommenting the `fig-dpi` in the yaml topmatter of the example will affect plots. (and it's set via code in point above).
* when run in a jupyterlab notebook, setting `theme(dpi=...)` worked fine

cc @cscheid, in case there's another important quarto piece I'm missing?! I'm a bit stumped here 😓 
cscheid commented 2 months ago

@machow You found the only thing I'm aware of, that the format fig-dpi setting is forwarded to rcParams exactly where you've highlighted it.

has2k1 commented 2 months ago

I have fixed this. Though it is worth noting that the combination of savefig.dpi and figure.dpi parameters is delicate, preferably only figure.dpi should be set.

https://github.com/quarto-dev/quarto-cli/issues/9470 is related to this in that we are still unable to get the expected display size.

machow commented 2 months ago

Thanks for the quick fix! I think the retina issue won't impact my work too much, since things like seaborn also generate images ~1200px wide, but with a width on the screen of ~600px (due to quarto page content layout width). (But I could see how using retina info is useful here)

cscheid commented 2 months ago

(I'm just piping in to say I love the issue shortcode for the changelog, and we might end up adopting something like that in quarto-cli ourselves!)

cscheid commented 2 months ago

I have fixed this. Though it is worth noting that the combination of savefig.dpi and figure.dpi parameters is delicate, preferably only figure.dpi should be set.

I know very little about matplotlib so I'm having a bit of trouble following the matplotlib issue. We'd be happy to change our defaults so that figure.dpi is set, but what are the consequences?

has2k1 commented 2 months ago

savefig.dpi is less used and usually users only set figure.dpi and want the same value for both cases. The way to express that intention without values going out-of-sync in edgecases is by setting the savefig.dpi to the string "figure".

 plt.rcParams['figure.dpi'] = fig_dpi
 plt.rcParams['savefig.dpi'] = "figure"

This way savefig.dpi will never go out-of-sync, unless the user knows what they are doing. It easily goes out-of-sync because the dpi of the figure can be altered (fig.set_dpi(42) or by saving the figure in a different rcParams context after it has been created!).

Using "figure" for savefig.dpi is how matplotlib decided to implement the "one source of truth", made it the default setting and therefore you can omit it.