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

Regression: Multi-part figures get stretched in recent versions #9927

Closed
Robinlovelace opened this issue Jun 8, 2024 · 21 comments · Fixed by #9940
Closed

Regression: Multi-part figures get stretched in recent versions #9927

Robinlovelace opened this issue Jun 8, 2024 · 21 comments · Fixed by #9940
Assignees
Labels
bug Something isn't working crossref regression Functionality that used to work but now is broken.
Milestone

Comments

@Robinlovelace
Copy link

Robinlovelace commented Jun 8, 2024

Bug description

Quarto creates outputs in which figures are distorted with layout-ncol values above one, e.g. in the following reproducible example:

#| label: fig-raster-slope
#| fig-cap: Slope and aspect calculation from a DEM
#| layout-ncol: 3
#| fig-subcap: 
#| - Input DEM
#| - Slope (degrees)
#| - Aspect (degrees)
# Input DEM
src_srtm = rasterio.open('output/srtm_32612.tif')
srtm = src_srtm.read(1).astype(float)
srtm[srtm == src_srtm.nodata] = np.nan
fig, ax = plt.subplots()
rasterio.plot.show(src_srtm, cmap='Spectral_r', ax=ax)
fig.colorbar(ax.imshow(srtm, cmap='Spectral_r'), ax=ax);
# Slope
src_srtm_slope = rasterio.open('output/srtm_32612_slope.tif')
srtm_slope = src_srtm_slope.read(1)
srtm_slope[srtm_slope == src_srtm_slope.nodata] = np.nan
fig, ax = plt.subplots()
rasterio.plot.show(src_srtm_slope, cmap='Spectral_r', ax=ax)
fig.colorbar(ax.imshow(srtm_slope, cmap='Spectral_r'), ax=ax);
# Aspect
src_srtm_aspect = rasterio.open('output/srtm_32612_aspect.tif')
srtm_aspect = src_srtm_aspect.read(1)
srtm_aspect[srtm_aspect == src_srtm_aspect.nodata] = np.nan
fig, ax = plt.subplots()
rasterio.plot.show(src_srtm_aspect, cmap='twilight', ax=ax)
fig.colorbar(ax.imshow(srtm_aspect, cmap='twilight'), ax=ax);

Steps to reproduce

See code chunk above.

To reproduce, follow steps here: https://github.com/geocompx/geocompy

You can also reproduce in a devcontainer hosted on GitHub:

Open in GitHub Codespaces

From there try typing quarto preview in the terminal.

Expected behavior

It should look like this, as it did in previous versions of Quarto:

image

Source: geocompx/geocompy#230 (comment)

Actual behavior

The figures are stretched

Your environment

  • IDE: VS Code. This has nothing to do with the IDE though.

Quarto check output

quarto check
Quarto 1.5.41
[✓] Checking versions of quarto binary dependencies...
      Pandoc version 3.2.0: OK
      Dart Sass version 1.70.0: OK
      Deno version 1.41.0: OK
      Typst version 0.11.0: OK
[✓] Checking versions of quarto dependencies......OK
[✓] Checking Quarto installation......OK
      Version: 1.5.41
      Path: /opt/quarto/bin

[✓] Checking tools....................OK
      TinyTeX: (not installed)
      Chromium: (not installed)

[✓] Checking LaTeX....................OK
      Tex:  (not detected)

[✓] Checking basic markdown render....OK

[✓] Checking Python 3 installation....OK
      Version: 3.11.4
      Path: /usr/local/bin/python3
      Jupyter: 5.7.2
      Kernels: python3

[✓] Checking Jupyter engine render....OK

[✓] Checking R installation...........(None)

      Unable to locate an installed version of R.
      Install R from https://cloud.r-project.org/
@Robinlovelace Robinlovelace added the bug Something isn't working label Jun 8, 2024
@Robinlovelace
Copy link
Author

See work-around here: geocompx/geocompy#233

Follow-on question, are there any good work-arounds on the quarto side?

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 8, 2024

Thanks for the report.

Could you share a small self-contained example as you seem to imply the code cell options snippet (not reproducible) is enough?

It's really important you strip out everything unrelated to s in your case (your GitHub repository) there are many possible cofounders.


You can share a self-contained "working" (reproducible) Quarto document using the following syntax, i.e., using more backticks than you have in your document (usually four ````).
See https://quarto.org/bug-reports.html#small-is-beautiful-aim-for-a-single-document-with-10-lines.

If you have multiple files (and if it is absolutely required to have multiple files), please share as a Git repository.

RPython
````qmd
---
title: "Reproducible Quarto Document"
format: html
engine: knitr
---

This is a reproducible Quarto document.

```{r}
x <- c(1, 2, 3, 4, 5)
y <- c(1, 4, 9, 16, 25)

plot(x, y)
```

![An image](https://placehold.co/600x400.png)

The end.
````
````qmd
---
title: "Reproducible Quarto Document"
format: html
engine: jupyter
---

This is a reproducible Quarto document.

```{python}
import matplotlib.pyplot as plt

x = [1, 2, 3, 4, 5]
y = [1, 4, 9, 16, 25]

plt.plot(x, y)
plt.show()
```

![An image](https://placehold.co/600x400.png)

The end.
````

Additionally and if not already given, please share the output of quarto check within a code blocks (i.e., using three backticks ```txt), see https://quarto.org/bug-reports.html#check.

@Robinlovelace
Copy link
Author

OK will try to create a minimal reprex..

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 8, 2024

Thanks!

@Robinlovelace
Copy link
Author

OK will try to create a minimal reprex..

Here you go:


---
title: "Reproducible Quarto Document"
format: html
engine: jupyter
---

This is a reproducible Quarto document.

```{python}
#| layout-ncol: 3

import rasterio
import numpy as np
import matplotlib.pyplot as plt
from rasterio.plot import show

# Example raster dataset: SRTM

src_srtm = rasterio.open('https://github.com/geocompx/geocompy/raw/main/data/srtm.tif')
srtm = src_srtm.read(1).astype(float)
srtm[srtm == src_srtm.nodata] = np.nan
fig, ax = plt.subplots()
rasterio.plot.show(src_srtm, cmap='Spectral_r', ax=ax)

```

The end.

@Robinlovelace
Copy link
Author

Robinlovelace commented Jun 8, 2024

Try rendering that doc. You will see something like this, if the bug persists on your computer (figure should be square):

image

I see no good reason for Quarto to stretch figures vertically like this without any instructions to do so or prior warning that it will. As per image above, it looked fine without the stretching.

@Robinlovelace
Copy link
Author

Robinlovelace commented Jun 8, 2024

How it should look is shown in the image in the original post: #9927 (comment)

That is how it does look on older versions of Quarto before this regression I think. We're not sure when the regression was introduced.

@cscheid
Copy link
Collaborator

cscheid commented Jun 8, 2024

You've requested layout-ncol: 3 here. What did you expect to happen?

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 8, 2024

You titled "regression", but compared to which version?

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 8, 2024

The markdown code emitted for the images contain an absolute size, so no resizing can happen responsively.
layout-ncol makes a constraint on the width but Quarto has no way to know what kind of content there is: it can be as simple as an image, but it can be a mix of anything thus you cannot also add a constraint on height.

If I set the height/width myself in your code:

Quarto documentHTML
---
format: html
engine: jupyter
keep-md: true
---

```{python}
#| layout-ncol: 3
import rasterio
import numpy as np
import matplotlib.pyplot as plt
from rasterio.plot import show

# Example raster dataset: SRTM

src_srtm = rasterio.open('https://github.com/geocompx/geocompy/raw/main/data/srtm.tif')
srtm = src_srtm.read(1).astype(float)
srtm[srtm == src_srtm.nodata] = np.nan
fig, ax = plt.subplots(figsize=(2, 2))
rasterio.plot.show(src_srtm, cmap='Spectral_r', ax=ax)

plt.show()
```
image

If you look at the intermediate markdown the image looks like this:

![](index_files/figure-html/cell-2-output-1.png){width=207 height=188}

The same thing without computations:

Quarto documentHTML
---
format: html
keep-md: true
---

::: {layout-ncol="3"}

![](https://placehold.co/450x450.png)

![](https://placehold.co/450x450.png)

![](https://placehold.co/450x450.png)

:::

---

::: {layout-ncol="3"}

![](https://placehold.co/450x450.png){width=450 height=450}

![](https://placehold.co/450x450.png)

![](https://placehold.co/450x450.png)

:::
image

@Robinlovelace
Copy link
Author

You titled "regression", but compared to which version?

We're not sure when the regression was introduced.

@Robinlovelace
Copy link
Author

You've requested layout-ncol: 3 here. What did you expect to happen?

I would expect the aspect ratio of the images not to altered, see the aspect ratio of the image below (zoomed in version of image in original post to highlight that this was also generated by Quarto):

image

Source: geocompx/geocompy#230 (comment)

In terms of the version of Quarto that generates that expected non-stretched version, I'm not sure but Michael might. @michaeldorman do you know what version of Quarto generated the image shown above? Thanks all.

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 8, 2024

If you have the HTML, the Quarto version is written as metadata in it.

@Robinlovelace
Copy link
Author

Robinlovelace commented Jun 8, 2024

- fig, ax = plt.subplots()
+ fig, ax = plt.subplots(figsize=(2, 2))

That looks like a good work-around. Would be great if the figures were not stretched without adding figsize arguments, as seems to have happened in older versions of Quarto.

@cscheid
Copy link
Collaborator

cscheid commented Jun 8, 2024

I've bisected it to a change from 1.4.366 to 1.4.367 - 1.4.367 is the FloatRefTarget release, somewhat unsurprisingly.

@cscheid cscheid self-assigned this Jun 8, 2024
@cscheid cscheid added crossref regression Functionality that used to work but now is broken. labels Jun 8, 2024
@cscheid cscheid added this to the v1.5 milestone Jun 8, 2024
@cscheid
Copy link
Collaborator

cscheid commented Jun 8, 2024

This is interesting, though. In the "good" versions, Quarto is completely removing the height attribute from the image as provided. The keep-md output is:

---
format: html
engine: jupyter
keep-md: true
---


::: {#dda85de0 .cell layout-ncol='3' execution_count=1}
``` {.python .cell-code}
import rasterio
import numpy as np
import matplotlib.pyplot as plt
from rasterio.plot import show

# Example raster dataset: SRTM

src_srtm = rasterio.open('https://github.com/geocompx/geocompy/raw/main/data/srtm.tif')
srtm = src_srtm.read(1).astype(float)
srtm[srtm == src_srtm.nodata] = np.nan
fig, ax = plt.subplots()
rasterio.plot.show(src_srtm, cmap='Spectral_r', ax=ax)

plt.show()
```

::: {.cell-output .cell-output-display}
![](9927_files/figure-html/cell-2-output-1.png){width=443 height=411}
:::
:::

Note {width=443 height=411} specifically.

The HTML output we produce in 1.4.366 (the "good" version) has

<p><img src="9927_files/figure-html/cell-2-output-1.png" class="img-fluid" width="443"></p>

Note how the height attribute got dropped.

I think the fundamental problem here is that it's impossible for Quarto to properly handle existing sizing constraints on images inside a layout-ncol declaration.

In a vacuum, what would be desired outcome here? layout-ncol:3 specifies the desire for three evenly-spaced columns, in which case each column has a max-width lower than the image. Because height is explicitly specified instead of aspect ratio, the web browser goes "cool, I'll keep the height while squeezing the width".

In 1.4.366, Quarto actively decides to drop the height of the images in a layout cell, see

transferImageWidthToCell(fig, cellDiv)

which calls

function transferImageWidthToCell(img, divEl)
divEl.attr.attributes["width"] = img.attributes["width"]
if sizeToPercent(attribute(img, "width", nil)) then
img.attributes["width"] = nil
end
img.attributes["height"] = nil
end

Note how line 134 drops the height.

We'll fix this soon.

@Robinlovelace
Copy link
Author

great job! Will look to install latest version to fix on our book also.

@Robinlovelace
Copy link
Author

Any ideas of ETA when there will be a release with this patch? Just looking here https://github.com/quarto-dev/quarto-cli/releases

@Robinlovelace
Copy link
Author

Hi @cscheid I'm sorry to report that the issue persists for us. As I said, it was not an issue in previous versions but it persists. Any suggestions to keep the aspect ratio unchanges please let us know. Happy to provide more details.

@Robinlovelace
Copy link
Author

Fully reproducible example inside a devcontainer:

quarto --version
1.5.47

image

@Robinlovelace
Copy link
Author

I've identified the cause here: subcaptions. Will open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working crossref regression Functionality that used to work but now is broken.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants