Skip to content

Add size keyword for figure dimensions (#77) - #3

Open
PabloBotin wants to merge 1 commit into
mainfrom
issue-77-figure-size
Open

Add size keyword for figure dimensions (#77)#3
PabloBotin wants to merge 1 commit into
mainfrom
issue-77-figure-size

Conversation

@PabloBotin

Copy link
Copy Markdown
Owner

Closes Sienna-Platform#77.

Context

Figure size was hardcoded at ext/plot_recipes.jl:14 — the only CairoMakie.Figure call in the repo — so every CairoMakie plot was 1280×720 with no way to change it. Because CairoMakie.save inherits the figure size, that also fixed every saved PNG at 2560×1440 (Makie applies px_per_unit = 2.0). PlotlyLight set no size at all.

Changes

  • src/definitions.jl — private DEFAULT_FIGURE_SIZE = (1280, 720), shared so the backends can't drift. Replaces a comment that misstated Makie's own default as 800×600; it is actually (600, 450).
  • ext/plot_recipes.jl_dataframe_plots_internal reads size and calls CairoMakie.resize!.
  • ext/plotly_recipes.jl — sets layout.width/layout.height.
  • src/call_plots.jl — one size bullet in each of the ten # Accepted Key Words blocks, matching how every other kwarg is documented here.
  • test/test_plot_creation.jl — figure-size testset running against both backends, plus an assertion on the demand path (the only exercised path that splats kwargs through a PowerAnalytics getter).

Design decision: PlotlyLight stays responsive by default

layout.width/height are set only when size is passed. PlotlyLight output is responsive to its container; defaulting it to a fixed 1280×720 would silently change the rendered size of every existing HTML plot. The backends therefore differ in their default but behave identically when size is given. This is called out in a code comment and in the docstring bullet so it doesn't read as an oversight.

Why it must work on the in-place path

Every public non-! entry point calls _empty_plot() with no arguments and immediately delegates to the ! form — the figure exists before kwargs are ever read. So size is applied on the in-place path; an earlier draft added a size keyword to _empty_plot, which was unreachable dead code and has been removed.

Verification

  • Full suite: 47/47, zero failures, zero Error log events. Formatter clean.
  • Break/revert, both backends: disabling the CairoMakie resize! → 3 failures; removing the PlotlyLight layout block → 3 failures. Reverted each time and confirmed the diff was byte-identical.
  • Real on-disk output (CairoMakie PNG, px_per_unit = 2.0): default → 2560×1440; size=(640,400) → 1280×800; in-place size=(500,300) → 1000×600.
  • PlotlyLight: haskey(layout, :width) and :height both false by default (responsive preserved); set correctly on both the fresh and in-place paths.
  • Independently reviewed, including an adversarial check that resize! does not fight Makie's legend rebuild or auto-limit reset — a 300×200 figure with a 25-entry legend holds its size, because Makie compresses the layout to fit rather than growing the canvas.

Known gaps

  • plot_fuel/plot_powerdata/plot_results with size are not covered by a test. plot_fuel is the only path that resizes twice in one call (fuel stack + net-load line); idempotence was verified manually but is not pinned in CI.
  • Nothing asserts that an in-place call without size leaves an earlier explicit size alone.
  • PlotlyLight sizing was verified in memory, not by inspecting the written HTML.

Adjacent pre-existing bug (not fixed here)

src/call_plots.jl advertises width, height, and scale as PlotlyLight save_plot kwargs, but SUPPORTED_PLOTLY_SAVE_KWARGS allowlists default_width/default_height instead — so all three documented names are silently dropped. Left for Sienna-Platform#91, which owns save-time sizing. The names px_per_unit, pt_per_unit, and save_size are deliberately unused here and reserved for that change.

Figure size was hardcoded at the single `CairoMakie.Figure` call site, and
PlotlyLight set no size at all, so output dimensions could not be controlled
from any plot function.

Add a `size::Tuple{Int, Int}` keyword, honored on both the fresh-figure and
in-place paths in both backends. CairoMakie resizes the figure, which
`CairoMakie.save` then inherits; PlotlyLight sets `layout.width`/`height`.

PlotlyLight is sized only when `size` is passed: its output is responsive to
the containing element, and defaulting it to a fixed size would change how
every existing HTML plot renders.

The shared default lives in `DEFAULT_FIGURE_SIZE`, replacing a comment that
misstated Makie's own default as 800x600 rather than 600x450.
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.

add kwarg for figure size

1 participant