Skip to content

Conversation

seisman
Copy link
Member

@seisman seisman commented Jul 14, 2025

This PR implements the Box class that can be used in modules like coast/image/legend/colorbar/....

The full GMT CLI syntax is (https://docs.generic-mapping-tools.org/6.6/colorbar.html#f):

-F[+cclearances][+gfill][+i[[gap/]pen]][+p[pen]][+r[radius]][+s[[dx/dy/][shade]]]

The upstream long-form names are available at:

https://github.com/GenericMappingTools/gmt/blob/7026382558e11f89e2238bb345e1d0af325c12aa/src/longopt/psscale_inc.h#L35

  • +c: clearance
  • +f: fill
  • +i: inner
  • +p: pen
  • +r: radius
  • +s: shade

The Box class follows the upstream long-form names, but use inner_gap/inner_pen for +i and shade_offset/shade_fill for +s.

Preview: https://pygmt-dev--3995.org.readthedocs.build/en/3995/api/generated/pygmt.params.Box.html

@seisman seisman force-pushed the AliasSystem/params/base branch 4 times, most recently from dd01578 to d9ccd1f Compare July 14, 2025 07:21
@seisman seisman force-pushed the AliasSystem/params/base branch from 311c83f to 926a690 Compare July 29, 2025 04:39
@seisman seisman added the feature Brand new feature label Aug 3, 2025
@seisman seisman added this to the 0.17.0 milestone Aug 3, 2025
@seisman seisman added the needs review This PR has higher priority and needs review. label Aug 3, 2025
@seisman seisman force-pushed the AliasSystem/params/base branch from db35954 to de1545f Compare August 3, 2025 06:04
@seisman seisman marked this pull request as ready for review August 3, 2025 06:04
@seisman seisman requested review from a team and Copilot August 24, 2025 08:04
Copilot

This comment was marked as outdated.

@seisman seisman requested a review from Copilot August 24, 2025 08:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new Box class to replace string-based box parameters throughout PyGMT with a more structured, object-oriented approach. The class provides type-safe parameter specification for GMT embellishment boxes with clear attribute names.

Key changes:

  • Added new Box class in pygmt.params.box with comprehensive parameter support
  • Created base class BaseParam for shared parameter functionality
  • Updated all existing box parameter usage from strings to Box instances across tests and examples

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pygmt/params/box.py Core implementation of the Box class with all GMT box parameters
pygmt/params/base.py Base class providing common parameter functionality and string conversion
pygmt/params/init.py Module initialization exposing the Box class
pygmt/tests/test_inset.py Updated inset tests to use Box class instead of strings
pygmt/tests/test_image.py Updated image test to use Box class
pygmt/src/inset.py Updated inset docstring example to use Box class
examples/tutorials/advanced/legends.py Updated legend examples to use Box class
examples/tutorials/advanced/insets.py Updated inset examples to use Box class
examples/gallery/lines/hlines_vlines.py Updated legend box to use Box class
examples/gallery/images/cross_section.py Updated colorbar box to use Box class
examples/gallery/embellishments/scalebar.py Updated scalebar box to use Box class
examples/gallery/embellishments/inset_rectangle_region.py Updated inset box to use Box class
examples/gallery/embellishments/inset.py Updated inset box to use Box class
doc/api/index.rst Added Box class to API documentation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@seisman seisman requested a review from a team August 26, 2025 14:37
@seisman
Copy link
Member Author

seisman commented Aug 29, 2025

@GenericMappingTools/pygmt-maintainers If you have time, please review this PR. You can pay more attention to the parameter names of the Box class, if you don't care much about the implementation details.

Comment on lines 34 to 43
inner_gap
Gap between the outer and inner borders. Default is ``"2p"``.
inner_pen
Pen attributes for the inner border. Default to :gmt-term:`MAP_DEFAULT_PEN`.
shading_offset
Place an offset background shaded region behind the box. A sequence of two
values (dx, dy) indicates the shift relative to the foreground frame. Default is
``("4p", "-4p")``.
shading_fill
Fill for the shading region. Default is ``"gray50"``.
Copy link
Member

Choose a reason for hiding this comment

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

We still have the rule to use underscores in parameter names only for bridging vocals (https://www.pygmt.org/latest/contributing.html#code-style). How strict do we want to be with this for introducing aliases for the modifiers? I personally feel that underscores make the aliases in general more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still have the rule to use underscores in parameter names only for bridging vocals (https://www.pygmt.org/latest/contributing.html#code-style).

If we follow the rule strictly, then the two parameter names would be shading_offset (with underscore) and shadingfill (without underscore). I feel it makes the names more confusing.

Comment on lines 96 to 101
@property
def _innerborder(self) -> list[str | float] | None:
"""
Inner border of the box, formatted as a list of 1-2 values, or None.
"""
return [v for v in (self.inner_gap, self.inner_pen) if v is not None] or None
Copy link
Member

Choose a reason for hiding this comment

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

Following the GMT docs for +s ([[gap/]pen]), in case gap is specified also pen has to be given. So, do we need any error or warning if inner_gap is set but inner_pen not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Done in 5ec1180. I'll add a test later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature needs review This PR has higher priority and needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants