-
Notifications
You must be signed in to change notification settings - Fork 230
Add the Box class for specifying the box of GMT embellishments #3995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dd01578
to
d9ccd1f
Compare
7aa3bf8
to
37ed709
Compare
Co-authored-by: Michael Grund <[email protected]> Co-authored-by: Yvonne Fröhlich <[email protected]>
311c83f
to
926a690
Compare
db35954
to
de1545f
Compare
There was a problem hiding this 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 inpygmt.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.
Co-authored-by: Yvonne Fröhlich <[email protected]>
@GenericMappingTools/pygmt-maintainers If you have time, please review this PR. You can pay more attention to the parameter names of the |
pygmt/params/box.py
Outdated
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"``. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pygmt/params/box.py
Outdated
@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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Yvonne Fröhlich <[email protected]>
This PR implements the
Box
class that can be used in modules likecoast
/image
/legend
/colorbar
/....The full GMT CLI syntax is (https://docs.generic-mapping-tools.org/6.6/colorbar.html#f):
The upstream long-form names are available at:
https://github.com/GenericMappingTools/gmt/blob/7026382558e11f89e2238bb345e1d0af325c12aa/src/longopt/psscale_inc.h#L35
The
Box
class follows the upstream long-form names, but useinner_gap
/inner_pen
for+i
andshade_offset
/shade_fill
for+s
.Preview: https://pygmt-dev--3995.org.readthedocs.build/en/3995/api/generated/pygmt.params.Box.html