Skip to content

feat(figure): add four-side outer margin to subplots (#311) - #312

Merged
SoichiroYamane merged 1 commit into
mainfrom
feat/311-subplots-margin
Oct 18, 2026
Merged

SoichiroYamane merged 1 commit into
mainfrom
feat/311-subplots-margin

Conversation

@SoichiroYamane

Copy link
Copy Markdown
Owner

Fixes #311.

Summary

gs.subplots(..., margin=(left, bottom, right, top)) reserves figure-edge space outside the constrained layout, fixing spine clipping with xpad=0.0, ypad=0.0 + crop=False. Pt values, fixed regardless of unit; internally sets the engine rect.

Changes

  • src/gsplot/_figure/layout.py: _margin validator (4-tuple, finite, non-negative); new parameter on all overloads + impl + signature fn + docstring; engine application with figure-covering and tight-layout rejection.
  • tests/unit/test_layout_foundation.py: rect mapping + 4 rejection paths.
  • tests/fixtures/reform/public-api-v1.json: regenerated (subplots signature/docstring only).
  • docs/guides/publication.md, docs/project/requirements.md: margin rule.

Compatibility classification

Additive: one optional parameter, default None preserves every existing call. No changes to pad/xpad semantics; no adapter needed (subplots is direct canonical).

Validation evidence

  • MPLBACKEND=Agg poetry run pytest -q tests: 509 passed.
  • black, isort, mypy, pyright src/gsplot, pyright examples, compileall, check_architecture.py, check_docstrings.py: clean.
  • sphinx-build -W -b html docs docs/_build/html: succeeded; no generated files in diff.
  • Live probe: margin reserves exact pt space, MediaBox unchanged at 246x300.
  • pip-audit --local: environment-blocked (no PyPI network); no dependency change in this slice.

Residual risks

  • layout='tight' + margin raises by design (no rect support there).
  • Reused figures with clear=False keep the caller's engine rect; caller owns that geometry.

Constrained layout offers only symmetric w_pad/h_pad, so xpad=0/ypad=0
with crop=False leaves spines flush on the media box. The new margin
(left, bottom, right, top) in points maps to the engine rect, coexists
with inner pads, and is rejected under tight layout or when it covers
the figure.
Copilot AI balanced review requested due to automatic review settings October 18, 2026 12:35
@SoichiroYamane

Copy link
Copy Markdown
Owner Author

Review 1 (requirements, API surface, compat, risk)

  • Linked Issue Add four-side outer margin control to subplots #311 records the decision (new margin param; order left/bottom/right/top; pt fixed; tight-layout rejection) with acceptance criteria; implemented exactly.
  • Public surface: one new optional parameter on subplots (4 overloads + impl + signature fn + docstring, all in sync). Frozen-contract diff limited to subplots records (verified: signature + docstring hashes only).
  • Compatibility: additive, default None is a no-op; pad/xpad/ypad semantics untouched; subplots is direct canonical so no root-adapter change needed (adapter has no subplots entry).
  • Risk: engine rect is caller-visible state on reused figures (documented residual); figure-covering margins rejected pre-mutation. No global-state, config, workflow, packaging, or dependency changes. No secrets or generated files (5 files). Push-time low Dependabot flag is pre-existing on main, unrelated.

@SoichiroYamane

Copy link
Copy Markdown
Owner Author

Review 2 (complete diff, checks, secrets/generated scan)

  • Final diff reviewed: layout.py (validator, plumbing, engine rect, docstring), test_layout_foundation.py (+2 tests), public-api-v1.json (subplots only), publication.md, requirements.md. Nothing else changed.
  • Checks (local, exact head 9225348): pytest 509 passed; black/isort/mypy/pyright(src+examples)/compileall/check_architecture/check_docstrings clean; Sphinx -W build succeeded; git diff --check clean. pip-audit environment-blocked, no dependency change.
  • Secrets/generated scan: no credentials, paths, or logs; no example outputs; docs/_build ignored.
  • Remote CI pending at review time; merge only after all required checks pass on this head.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SoichiroYamane

Copy link
Copy Markdown
Owner Author

CI green on head 9225348: all required checks pass (tests 3.10/3.12, documentation incl. versioned build, format, audit, CodeQL). Ready to merge; awaiting maintainer approval.

@SoichiroYamane
SoichiroYamane merged commit 81f0036 into main Oct 18, 2026
13 checks passed
@SoichiroYamane
SoichiroYamane deleted the feat/311-subplots-margin branch October 18, 2026 13:21
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 four-side outer margin control to subplots

2 participants