Respect explicit figure sizes in tests

plotnine/_utils/yippie.py

@@ -40,7 +40,6 @@ def __getattr__(self, color: str):
                 title=color.title(),
             )
             + theme(
-                figure_size=(8, 6),
                 text=element_text(color="black", size=11),
                 panel_background=element_rect(fill=color, size=1),
                 plot_background=element_rect(fill=color, alpha=0.2),

plotnine/composition/_inset_image.py

@@ -123,7 +123,7 @@ def _arrange_in_box(
             self.figure,
             anchor=self._anchor,
         )
-        self._frac_bbox.bounds = (l, b, r - l, t - b)  # pyright: ignore[reportAttributeAccessIssue]
+        self._frac_bbox.bounds = (l, b, r - l, t - b)
         self.patch.set_bounds(left, bottom, right - left, top - bottom)
 
         # The layout engine has finalised the bbox, so its device-pixel

plotnine/geoms/geom_ribbon.py

@@ -142,7 +142,7 @@ def draw_unit(
             _x,
             _min,
             _max,
-            where=where,  # type: ignore
+            where=where,
             interpolate=interpolate,
             facecolor=fill,
             edgecolor=color,

pyproject.toml

@@ -88,7 +88,7 @@ dev = [
 ]
 
 typing = [
-    "pyright==1.1.410",
+    "pyright==1.1.411",
     "ipython",
     "pandas-stubs",
 ]
@@ -207,6 +207,14 @@ stubPath = ""
 ########## Tool - Ruff ##########
 [tool.ruff]
 line-length = 79
+# Exclude a variety of commonly ignored directories.
+exclude = [
+    "plotnine/themes/seaborn_rcmod.py",
+    "**/__pycache__",
+    "node_modules",
+    "README.md",
+]
+
 
 [tool.ruff.lint]
 select = [
@@ -241,14 +249,6 @@ ignore = [
 fixable = ["ALL"]
 unfixable = []
 
-# Exclude a variety of commonly ignored directories.
-exclude = [
-    "plotnine/themes/seaborn_rcmod.py",
-    "**/__pycache__",
-    "node_modules"
-]
-
-
 # Allow unused variables when underscore-prefixed.
 dummy-variable-rgx = "^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$"
 

tests/conftest.py

@@ -18,6 +18,7 @@
     inset_element,
     plot_annotation,
 )
+from plotnine.options import set_option
 from plotnine.themes.theme import DEFAULT_RCPARAMS
 
 TOLERANCE = 2  # Default tolerance for the tests
@@ -28,11 +29,14 @@
 DEFAULT_RCPARAMS["font.sans-serif"] = ["Dejavu Sans"]
 DEFAULT_RCPARAMS["font.serif"] = ["Dejavu Serif"]
 
-# This partial theme modifies all themes that are used in
-# the test. It is limited to setting the size of the test
-# images Should a test require a larger or smaller figure
-# size, the dpi or aspect_ratio should be modified.
-test_theme = theme(figure_size=(640 / DPI, 480 / DPI), dpi=DPI)
+# Set figure size and DPI through package options so plot themes can override
+# them. Tests that omit either setting retain a 640-by-480-pixel image at
+# 72 DPI. Configure the options before test modules construct their themes.
+set_option("dpi", DPI)
+set_option("figure_size", (640 / DPI, 480 / DPI))
+
+# Give compositions and standalone insets more room than a single plot.
+default_composition_theme = theme(figure_size=(8, 6), dpi=DPI)
 
 tests_dir = Path(__file__).parent
 baseline_images_dir = tests_dir / "baseline_images"
@@ -70,7 +74,6 @@ def ggplot_equals(plot: ggplot, name: str) -> bool:
     # Save the figure before testing whether the original image
     # actually exists. This makes creating new tests much easier,
     # as the result image can afterwards just be copied.
-    plot += test_theme
     with _test_cleanup():
         plot.save(filenames.result, verbose=False)
 
@@ -240,7 +243,22 @@ def composition_equals(cmp: Compose, name: str) -> bool:
     test_file = inspect.stack()[1][1]
     filenames = make_test_image_filenames(name, test_file)
 
-    _cmp = cmp + plot_annotation(theme=theme(figure_size=(8, 6), dpi=DPI))
+    # Apply the test default after any complete theme broadcast with `&`. Its
+    # default figure size is indistinguishable from an explicit choice.
+    # Preserve a size set on a partial annotation theme because it explicitly
+    # sizes the composition.
+    annotation_theme = cmp.annotation.theme
+    declares_own_size = (
+        not annotation_theme.complete
+        and annotation_theme.getp("figure_size") is not None
+    )
+    composition_theme = (
+        default_composition_theme + annotation_theme
+        if declares_own_size
+        else default_composition_theme
+    )
+
+    _cmp = cmp + plot_annotation(theme=composition_theme)
 
     with _test_cleanup():
         _cmp.save(filenames.result)
@@ -286,7 +304,7 @@ def inset_element_equals(inset: inset_element, name: str) -> bool:
     test_file = inspect.stack()[1][1]
     filenames = make_test_image_filenames(name, test_file)
 
-    host = inset._blank_host + theme(figure_size=(8, 6), dpi=DPI)
+    host = inset._blank_host + default_composition_theme
     with _test_cleanup():
         (host + inset).save(filenames.result, verbose=False)
 

tests/test_layout.py

@@ -136,7 +136,7 @@ def test_plot_margin_aspect_ratio(self):
         p = (
             ggplot()
             + geom_blank()
-            + theme(plot_margin=0.025, figure_size=(4, 3))
+            + theme(plot_margin=0.025, figure_size=(8, 4))
         )
         assert p == "plot_margin_aspect_ratio"
 

tools/visualize_tests.py

@@ -7,6 +7,7 @@
 #
 
 import argparse
+import struct
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Iterator
@@ -41,6 +42,20 @@ def highlight_python(source: str) -> str:
         return escape(source)
 
 
+def png_size(path: Path) -> tuple[int, int] | None:
+    """
+    Read the pixel width and height of a PNG file
+
+    Returns `None` if the file is not a PNG or is truncated.
+    """
+    with path.open("rb") as f:
+        header = f.read(24)
+    if len(header) < 24 or header[:8] != b"\x89PNG\r\n\x1a\n":
+        return None
+    width, height = struct.unpack(">II", header[16:24])
+    return width, height
+
+
 @dataclass(frozen=True)
 class TestImage:
     """A single image-comparison test result"""
@@ -50,10 +65,23 @@ class TestImage:
     actual_rel: str
     expected_rel: str | None
     failed_rel: str | None
+    actual_size: tuple[int, int] | None
+    expected_size: tuple[int, int] | None
+
+    @property
+    def size_mismatch(self) -> bool:
+        """Whether the two images have different pixel dimensions"""
+        return (
+            self.actual_size is not None
+            and self.expected_size is not None
+            and self.actual_size != self.expected_size
+        )
 
     @property
     def status(self) -> str:
-        if self.failed_rel:
+        # The test framework compares pixels, so differently sized images
+        # fail without producing a diff image.
+        if self.failed_rel or self.size_mismatch:
             return "failed"
         if self.expected_rel is None:
             return "new"
@@ -98,6 +126,10 @@ def get_test_images() -> Iterator[TestImage]:
                 failed_rel=(
                     f"{subdir.name}/{failed.name}" if failed.exists() else None
                 ),
+                actual_size=png_size(png),
+                expected_size=(
+                    png_size(expected) if expected.exists() else None
+                ),
             )
 
 
@@ -459,6 +491,35 @@ def get_test_images() -> Iterator[TestImage]:
   border-radius: 4px;
 }
 
+.images figcaption .dims {
+  text-transform: none;
+  letter-spacing: 0;
+  font-variant-numeric: tabular-nums;
+}
+
+/* Scaling both images to the column width would hide the very
+   difference this view exists to show. */
+.images-natural figure {
+  flex: 0 0 auto;
+  min-width: 0;
+  max-width: 100%;
+}
+
+.images-natural img {
+  width: auto;
+  max-width: 100%;
+}
+
+.note {
+  margin: 0;
+  padding: 6px 10px;
+  border: 1px solid var(--red);
+  border-radius: 6px;
+  background: var(--red-soft);
+  color: var(--red);
+  font-size: 12px;
+}
+
 .test-row.view-side .images-slider,
 .test-row.view-side .images-flip,
 .test-row.view-slider .images-side,
@@ -923,7 +984,7 @@ def get_test_images() -> Iterator[TestImage]:
       const cap = fig.querySelector('figcaption');
       if (img && cap) {
         sibs.push({
-          label: cap.textContent.trim(),
+          label: fig.dataset.label || cap.textContent.trim(),
           src: img.getAttribute('src'),
         });
       }
@@ -1103,14 +1164,43 @@ def get_test_images() -> Iterator[TestImage]:
 """
 
 
-def _img_cell(caption: str, src: str) -> str:
+def _img_cell(
+    caption: str, src: str, size: tuple[int, int] | None = None
+) -> str:
+    dims = f' <span class="dims">{size[0]}×{size[1]}</span>' if size else ""
     return (
-        f"<figure><figcaption>{caption}</figcaption>"
+        f'<figure data-label="{caption}">'
+        f"<figcaption>{caption}{dims}</figcaption>"
         f'<a href="{src}"><img src="{src}" loading="lazy" alt=""></a>'
         f"</figure>"
     )
 
 
+def _size_mismatch_content(test: TestImage) -> str:
+    """
+    Content for a test whose images cannot be compared pixel-by-pixel
+
+    Both images are shown at their natural size, so the difference in
+    dimensions is visible rather than scaled away.
+    """
+    assert test.actual_size is not None
+    assert test.expected_size is not None
+    aw, ah = test.actual_size
+    ew, eh = test.expected_size
+    note = (
+        f'<p class="note">Image sizes differ: actual {aw}×{ah}, '
+        f"expected {ew}×{eh}. There is no pixel diff.</p>"
+    )
+    return (
+        '<div class="content">'
+        + note
+        + '<div class="images images-side images-natural">'
+        + _img_cell("actual", test.actual_rel, test.actual_size)
+        + _img_cell("expected", test.expected_rel or "", test.expected_size)
+        + "</div></div>"
+    )
+
+
 def _failed_content(test: TestImage) -> str:
     actual = test.actual_rel
     expected = test.expected_rel or ""
@@ -1251,7 +1341,9 @@ def render_row(test: TestImage) -> str:
         )
     meta = '<div class="meta">' + "".join(parts) + "</div>"
 
-    if test.status == "failed":
+    if test.size_mismatch:
+        content = _size_mismatch_content(test)
+    elif test.status == "failed":
         content = _failed_content(test)
     elif test.status == "new":
         content = _new_content(test)