-
Notifications
You must be signed in to change notification settings - Fork 233
Add the Position class for GMT embellishment placement #4212
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
pygmt/params/position.py
Outdated
|
|
||
| #: Specify the reference point on the plot. The method of defining the reference | ||
| #: point is controlled by ``type``, and the exact location is set by ``position``. | ||
| location: Sequence[float | str] | AnchorCode |
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.
position or location?
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.
I think location is better as the parameter is already called position and we overall will have the Position class.
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.
The docstring above calls this 'reference point', so how about something like ref or refpt?
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.
Maybe refpoint?
In fact, I'd prefer to make it a positional-only parameter, so it will be used like Position("TL", type="inside"), Position((1, 2)).
In this case, the specific parameter name doesn't really matter, though dataclasses doesn't support positional-only attributes.
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.
Yeah, I did think of positional-only, but a little tricky with dataclasses as you said. Ok to got with refpoint, but let's see what @yvonnefroehlich thinks.
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.
I am fine with refpoint. Unfortunately, I have only little experience with positional-only parameters.
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 the Position class to provide a structured, Pythonic way to specify positions for GMT embellishments (logo, scale, rose, etc.). The class translates between PyGMT's long-form parameters and GMT's position syntax string format.
Key Changes:
- Implements
Positionclass supporting five coordinate systems (map, plot, box, inside, outside) - Provides validation for location, type, anchor, and offset parameters
- Includes comprehensive test coverage for various position specifications
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
pygmt/params/position.py |
New Position class implementation with validation and GMT string conversion |
pygmt/tests/test_params_position.py |
Test suite covering position types, anchor/offset combinations, and validation |
pygmt/params/__init__.py |
Exports the new Position class |
doc/api/index.rst |
Adds Position to the API documentation index |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Ping @GenericMappingTools/pygmt-maintainers for final reviews. I plan to merge it in 48 hours if no further comments. |
| .. figure:: https://docs.generic-mapping-tools.org/dev/_images/GMT_anchor.png | ||
| :width: 600 px | ||
| :align: center |
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.
I feel we should make the images here and the one in the Technical References at https://pygmt-dev--4212.org.readthedocs.build/en/4212/techref/justification_codes.html consistent regarding the used colors.
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.
Here we use the image from the GMT documentation, mainly because it's technically difficult to generate an image dynamically in docstrings.
pygmt/params/position.py
Outdated
|
|
||
| **Reference Point** | ||
|
|
||
| The *reference point* can be specified in five different ways using the ``type`` and |
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.
I am unsure about type as attribute name, as type or type() seems to be a reserved keyword or function by Python itself:
We often use the name of a parameter or attribute for the name of the variable passed to the parameter or attribute:
import pygmt
from pygmt.params import Position
v = 5
# %%
type(v)
# works and outputs int
fig = pygmt.Figure()
fig.basemap(region=[0, 10, 0, 10], projection="X10c", frame=True)
fig.logo(position=Position("TL", type="inside", offset="0.2c"), box=True)
fig.show()
# %%
type = "inside"
type(v)
# fails as function was overwritten
fig = pygmt.Figure()
fig.basemap(region=[0, 10, 0, 10], projection="X10c", frame=True)
fig.logo(position=Position("TL", type=type, offset="0.2c"), box=True)
fig.show()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.
Yes, type is not an ideal name. Several options are:
coord_type: accurate but a little too longkind: a general term, similar totypecs_type: cs is short for "coordinate system", not that bad.
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.
I am fine with coord_type or cs_type. coord_type is more descriptive, but I have a slight preference for cs_type, as I feel for the arguments inside and outside "coordinate system" makes more sense. As geopandas uses crs for "coordinate reference systems", cs for "coordinate system" should be fine.
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.
Maybe cstype, since cs is already a shorthand for coordinate system?
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
See #4168 (comment) for context.
This PR implements the
Positionclass.Usage:
GMT Documentation: https://docs.generic-mapping-tools.org/dev/reference/features.html#reference-and-anchor-point-specification
Preview: https://pygmt-dev--4212.org.readthedocs.build/en/4212/api/generated/pygmt.params.Position.html