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 display of images created by the jupyter engine #9538

Merged
merged 4 commits into from
May 3, 2024

Conversation

has2k1
Copy link
Contributor

@has2k1 has2k1 commented Apr 30, 2024

This PR makes quarto render Jupyter generated images to the same size as they are rendered in Jupyter native environments like jupyter-lab or notebook.

  1. 25c725b fixes an edge case for matplotlib images where the DPI of a displayed figure may not match the DPI with which the image is saved; contrary to the intention of the implementation.
  2. f835a76 makes quarto consider the display width & height of generated images (if jupyter/ipython has set them) before falling back to the "fluid" image size.

References: has2k1/plotnine#773, #9470

@cscheid
Copy link
Collaborator

cscheid commented Apr 30, 2024

We also need tests that check for the output so we don't regress later. You can pattern-match on the examples we have in tests/docs/smoke-all. You'll want to add some entries like

_quarto:
  tests:
    html:
      ensureFileRegexMatches:
        ...
      ensureHtmlElements:
        ...

Use ensureFileRegexMatches if it's easier to check with a regex, but more prefereably, use ensureHtmlElements which will check using document.querySelector.

@cscheid
Copy link
Collaborator

cscheid commented Apr 30, 2024

Tests pass 🎉 - let me know if you need help with the smoke-all stuff or the .ts fixes ahead of merging.

Making plots with matplotlib, the dpi of the displayed image (`figure.dpi`)
can be controlled independently of the dpi with which it is saved
(`savefig.dpi`).

If you want the same value for the two cases the recommended way to
do it is to set `figure.dpi` to the desired numeric value, then have
`savefig.dpi` reference the `figure.dpi`. This is a achieved with

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

Is also the default value in matplotlib.

When `savefig.dpi` is set to the same numeric value as `figure.dpi`, it
may not apply because the matplotlib figure can be saved (and it is often
useful to do so) outside the context in which it was created; in which case
it would refer to another `savefig.dpi` value.

Ref: has2k1/plotnine#773
@cscheid
Copy link
Collaborator

cscheid commented May 1, 2024

(@has2k1 I added a merge commit for the changelog so that we can start running the test suite)

Those tests look good, thank you!

has2k1 added 3 commits May 1, 2024 20:06
First this commit fixes an error where `"key"` is used instead of `key`.

Images created by ipython display functions(which includes matplotlib
plots) may have the desired display width and height in the metadata.
When they do, we fallback to those values instead of none.

After this commit, the display size of images in jupyter notebooks
that are rendered by quarto should be exactly the same as the sizes
in their original environment.

fixes quarto-dev#9470
let width = metadataValue(kCellOutWidth, 0);
let height = metadataValue(kCellOutHeight, 0);
let width = metadataValue(kCellOutWidth, "width", 0);
let height = metadataValue(kCellOutHeight, "height", 0);
Copy link
Contributor Author

@has2k1 has2k1 May 1, 2024

Choose a reason for hiding this comment

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

@cderv For the Jupyter kernel, I couldn't think of a case were the metadata would contain KCellOutWidth="out-width" & KCellOutHeight="out-height". Is this for future support?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed this. I don't know exactly - I'll look closer. Maybe it was placeholder in anticipation to bring out-width and out-height to Jupyter (as it is supported in knitr initially)

@cscheid cscheid merged commit 78fc0b7 into quarto-dev:main May 3, 2024
47 checks passed
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.

Retina images generated using ipython display functions do not have their dimensions set
3 participants