Skip to content
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

Figure.coast: Add alias "box" for "-F" #2823

Merged
merged 11 commits into from
Dec 8, 2023
Merged

Figure.coast: Add alias "box" for "-F" #2823

merged 11 commits into from
Dec 8, 2023

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Nov 19, 2023

Description of proposed changes

This PR aims to add the alias box for -F of pygmt.Figure.coast:

Preview: https://pygmt-dev--2823.org.readthedocs.build/en/2823/api/generated/pygmt.Figure.coast.html

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@yvonnefroehlich yvonnefroehlich added the enhancement Improving an existing feature label Nov 19, 2023
@yvonnefroehlich yvonnefroehlich added this to the 0.11.0 milestone Nov 19, 2023
@yvonnefroehlich yvonnefroehlich self-assigned this Nov 19, 2023
@yvonnefroehlich yvonnefroehlich changed the title Figure.coast: Add alias "box" for "-F" WIP: Figure.coast: Add alias "box" for "-F" Nov 19, 2023
@seisman
Copy link
Member

seisman commented Nov 20, 2023

  • Do we want to add this docstring to decorators.py? Have to check whether this makes sense. There are other methods that also have the box parameter, but there are probably differences in the functionality and syntax.

Preview:

I believe the box parameter has different syntax in different modules (like basemap, image and coast).

@seisman
Copy link
Member

seisman commented Nov 26, 2023

  • Do we want a consistent alphabetical order?

I think we're sorting the single-letter options alphabetically in most module wrappers.

@yvonnefroehlich
Copy link
Member Author

  • Do we want a consistent alphabetical order?

I think we're sorting the single-letter options alphabetically in most module wrappers.

For coast, this is done in commit a32e08a. Should I submit a separate PR to make this consistent accross als functions and methods?

@seisman
Copy link
Member

seisman commented Nov 26, 2023

  • Do we want a consistent alphabetical order?

I think we're sorting the single-letter options alphabetically in most module wrappers.

For coast, this is done in commit a32e08a. Should I submit a separate PR to make this consistent accross als functions and methods?

Sorry, it seems that we still haven't reached an agreement about the ordering of aliases. See #884.

@yvonnefroehlich
Copy link
Member Author

  • Do we want a consistent alphabetical order?

I think we're sorting the single-letter options alphabetically in most module wrappers.

For coast, this is done in commit a32e08a. Should I submit a separate PR to make this consistent accross als functions and methods?

Sorry, it seems that we still haven't reached an agreement about the ordering of aliases. See #884.

No worries, thanks for pointing out this issue! I will have a more detailed look at it in the next days.

@seisman
Copy link
Member

seisman commented Dec 7, 2023

@yvonnefroehlich What's the status of this PR?

@yvonnefroehlich
Copy link
Member Author

@yvonnefroehlich What's the status of this PR?

So far I simply copied the docstring for the box parameter of Figure.basemap and did not compare https://docs.generic-mapping-tools.org/dev/basemap.html#f vs. https://docs.generic-mapping-tools.org/dev/coast.html#f. However, if people know that the provided functionaly is identical, this PR should be ready for review.

@yvonnefroehlich yvonnefroehlich changed the title WIP: Figure.coast: Add alias "box" for "-F" Figure.coast: Add alias "box" for "-F" Dec 7, 2023
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Dec 7, 2023
Comment on lines +132 to +141
box : bool or str
[**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\ *pen*]]\
[**+p**\ [*pen*]][**+r**\ [*radius*]][**+s**\ [[*dx*/*dy*/][*shade*]]].
If set to ``True``, draw a rectangular border around the
map scale or rose. Alternatively, specify a different pen with
**+p**\ *pen*. Add **+g**\ *fill* to fill the scale panel [Default is
no fill]. Append **+c**\ *clearance* where *clearance* is either gap,
xgap/ygap, or lgap/rgap/bgap/tgap where these items are uniform,
separate in x- and y-direction, or individual side spacings between
scale and border. Append **+i** to draw a secondary, inner border as
Copy link
Member Author

Choose a reason for hiding this comment

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

In the function string we have (plural)

[**+c**\ *clearances*]

but in the docstring we write (singular):

**+c**\ *clearance*

This is also the case in the upstream GMT documentation, as there is made a difference between "a uniform clearance" and "separate / individual clearances" (please see https://docs.generic-mapping-tools.org/dev/coast.html#f). But in the PyGMT docstring we do not have these terms, and maybe we should use either singular or plural, even it is not consistent with the GMT docs?

Copy link
Member

Choose a reason for hiding this comment

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

Using plural makes more sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to "clearances" (plural) for Figure.basemap and Figure.coast in commit 9be73a9.

But there are actually more affected docstrings. Maybe we should move this change to a separate PR and focus in this PR on adding the alias box for -F?

(base) yfroe@gpiseis16:~/Documents/01_GitHub/github_pygmt/pygmt/pygmt/src> grep "clearance" *
basemap.py:        [**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\ *pen*]]\
basemap.py:        no fill]. Append **+c**\ *clearance* where *clearance* is either gap,
colorbar.py:        [**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\ *pen*]]\
colorbar.py:        **+c**\ *clearance* where *clearance* is either gap, xgap/ygap, or
image.py:        [**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\ *pen*]]\
inset.py:        [**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\
inset.py:        Append **+c**\ *clearance* where *clearance* is either
inset.py:        This is clearance that is added around the inside of the inset.
legend.py:        [**+c**\ *clearances*][**+g**\ *fill*][**+i**\ [[*gap*/]\ *pen*]]\
grep: __pycache__: Is a directory
subplot.py:    C="clearance",
subplot.py:        **+c**\ *dx*\[/*dy*] to set the clearance between the tag and a
subplot.py:    clearance : str or list
subplot.py:        [*side*]\ *clearance*.
subplot.py:        Reserve a space of dimension *clearance* between the margin and the
subplot.py:        **s** and **n**. No *side* means all sides (i.e. ``clearance="1c"``
subplot.py:        would set a clearance of 1 cm on all sides). The option is repeatable
subplot.py:        ``clearance=["w1c", "s2c"]`` would set a clearance of 1 cm on west
subplot.py:        ``clearance`` to secure extra space for long horizontal annotations.
subplot.py:@use_alias(A="fixedlabel", C="clearance", V="verbose")
subplot.py:    clearance : str or list
subplot.py:        [*side*]\ *clearance*.
subplot.py:        Reserve a space of dimension *clearance* between the margin and the
subplot.py:        than one side (e.g. ``clearance=["w1c", "s2c"]`` would set a clearance
subplot.py:        clearances set by ``clearance`` in the initial
text.py:    C="clearance",
text.py:    clearance : str
text.py:        Adjust the clearance between the text and the surrounding box
text.py:        (see ``clearance``) [Default is ``"0.25p,black,solid"``].

Copy link
Member

Choose a reason for hiding this comment

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

I feel these are just minor issues and aren't worth the time fixing them. On the other hand, it's likely that we need to fully revise these docstrings towards a Pythonic interface (#1082 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Then we leave this as is and this PR should be finished. I revert commit 9be73a9.

@seisman seisman merged commit 1e48282 into main Dec 8, 2023
17 checks passed
@seisman seisman deleted the add-coast-alias-F branch December 8, 2023 13:46
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants