Honor width/height/scale when saving plots (#91) - #4
Open
PabloBotin wants to merge 1 commit into
Open
Conversation
…rm#91) The docstring advertised `width`, `height`, and `scale` as `save_plot` keywords, but neither backend implemented them. CairoMakie's `save_plot` accepted `kwargs...` and discarded every one; PlotlyLight filtered them against an allowlist of PlotlyBase names it could not forward anyway. Implement all three. On CairoMakie `width`/`height` set the saved scene size and `scale` multiplies `px_per_unit` (png) and `pt_per_unit` (pdf/svg), so `scale = 1` reproduces the previous output exactly. On PlotlyLight `width`/`height` set the Plotly layout and `scale` warns and is ignored, since HTML has no rendering scale. Both backends restore the plot object afterwards, including when the write throws: `Makie.save` resizes the scene and never restores it, so passing a size through would otherwise permanently resize the caller's figure. Also drop `SUPPORTED_PLOTLY_SAVE_KWARGS` and stop forwarding keywords to `show(io, MIME("text/html"), plot)`, which accepts none — any allowlisted keyword raised a `MethodError`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes Sienna-Platform#91.
Addresses kdayday's comment on the issue: "width/height/scale not available for resizing PNG/PDF/SVG at save time."
Three defects on
mainsave_plotacceptedkwargs...and discarded all of them — the body wasCairoMakie.save(filename, plot.figure). No size, no DPI control.width,height,scale, butSUPPORTED_PLOTLY_SAVE_KWARGSallowlisteddefault_width/default_height/autoplay/post_script/full_html/animation_opts. All three documented names were silently filtered out — a live documentation lie.show(io, MIME("text/html"), plot; save_kwargs...). PlotlyLight'sshowaccepts no keyword arguments, so any allowlisted name raised aMethodError. Latent only because nothing ever passed one.Changes
width/heightset the saved scene size;scalemultipliespx_per_unit(png) andpt_per_unit(pdf/svg). Only one supplied dimension is completed from the figure's current size. Validation via multiple dispatch; bad input raisesArgumentError.width/heightset the Plotly layout;scalewarns and is ignored, since HTML has no rendering scale.SUPPORTED_PLOTLY_SAVE_KWARGSdeleted andshowcalled with no keywords, fixing defect 3. The.htmlextension check is now case-insensitive, matching the CairoMakie side.src/call_plots.jl— docstring corrected to describe what the code does, including thatwidth/heightare Makie scene units on CairoMakie (sowidth = 800writes a 1600 px PNG at the defaultpx_per_unit = 2).scale = 1reproduces the previous output byte-for-byte, and the no-keyword path is unchanged.The trap this had to avoid
Makie.save(...; size = …)callsresize!(scene, size)and never restores it — and it resizes before its owntry, then rethrows without unwinding. Passing a size straight through would have permanently resized the caller's liveFigure; every laterdisplayand every later save would inherit the new size. Both backends therefore snapshot and restore in afinally, so the plot object is unchanged even when the write throws.Reading a
PlotlyLightlayout key also had to be done carefully: EasyConfig'sgetpropertyauto-vivifies on read, soplot.layout.widthon an unset layout inserts an emptyConfig()that serializes as"width":{}. Every read ishaskey-guarded, and keys absent beforehand are deleted on restore.Verification
Full suite 65/65, zero Error log events; formatter clean.
Directly exercised against the real API — 21/21:
px_per_unit=2)width=800, height=600+ scale=2width=800only"width":{}artifactfull_html = trueMethodError.htmlon CairoMakie /width=0/scale=-1ArgumentErrorsizepassed tosave_plotscalewas confirmed to reach vector formats too: SVGwidth="400"→width="1200", PDF MediaBox300pt→900ptatscale=3.Known limitations
scaleoverrides a custom Makie theme. The 2.0/0.75 multipliers are Makie's documented defaults, hardcoded rather than read fromCURRENT_DEFAULT_THEME(an undocumented internal). WithCairoMakie.activate!(px_per_unit = 1.0), the no-scalepath respects the theme but passingscalere-imposes the default. Documented in the docstring rather than handled.width = trueis accepted (Bool <: Integer) and writes a 2 px image. Absurd input, harmless outcome.scaleis only smoke-tested in CI (filesize > 0); the dimension change was verified manually, not pinned.<div>fragment rather than a standalone page (pre-existing). Consequentlyfull_html = truenow silently does nothing instead of crashing — strictly better, but the keyword was never functional.Adjacent, not fixed here
_plot_demand!reads:savewithout popping it and forwards the full kwargs onward, soplot_demand(...; save = dir)appears to write the same file twice. Pre-existing and unrelated; the restore logic makes it harmless.