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.plot/Figure.plot3d: Improve the docstrings for straight_line #3720

Merged
merged 12 commits into from
Jan 6, 2025

Conversation

seisman
Copy link
Member

@seisman seisman commented Dec 25, 2024

See the upstream issue report GenericMappingTools/gmt#8645 for context.

Here is an exmaple to prove that m|y|r and p|x|t have the same effects:

import pygmt
fig = pygmt.Figure()
values = [False, True, "m", "p", "r", "t", "x", "y"]
for arg in values:
    fig.basemap(region=[0, 360, -90, 90], projection="H6c", frame=[f"WSen+t-A={arg}"])
    fig.plot(x=[90, 200], y=[-50, 50], pen="1p", straight_line=arg)
    fig.shift_origin(xshift="w+1")

fig.shift_origin(xshift="-8w-8", yshift="h+3")

for arg in values:
    fig.basemap(region=[0, 10, 1, 10], projection="X6c", frame=[f"WSen+t-A={arg}"])
    fig.plot(x=[3, 8], y=[2, 7], pen="1p", straight_line=arg)
    fig.shift_origin(xshift="w+1")

fig.shift_origin(xshift="-8w-8", yshift="h+3")

for arg in values:
    fig.basemap(region=[0, 360, 0, 1], projection="P6c", frame=[f"WSen+t-A={arg}"])
    fig.plot(x=[0, 90], y=[0.2, 0.5], pen="1p", straight_line=arg)
    fig.shift_origin(xshift="w+1")
fig.show()

straight_line


The upstream documentation will simplify plot/plot3d's -A[m|p|r|t|x|y] to -A[x|y], and we will do the similar thing in the PyGMT side.

This PR rephrases the docstrings for straight_line.

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

TODO:

  • Polish and finalize the docstrings for straight_line
  • Make the same change for Figure.plot3d
  • Apply the same changes (with minor changes) to the upstream documentation

@seisman seisman added the help wanted Helping hands are appreciated label Dec 25, 2024
@seisman seisman added documentation Improvements or additions to documentation skip-changelog Skip adding Pull Request to changelog labels Dec 27, 2024
@seisman seisman added this to the 0.14.0 milestone Dec 27, 2024
@seisman seisman marked this pull request as ready for review December 27, 2024 03:55
@seisman seisman added the needs review This PR has higher priority and needs review. label Dec 27, 2024
@seisman seisman mentioned this pull request Dec 27, 2024
49 tasks
pygmt/src/plot.py Outdated Show resolved Hide resolved
Comment on lines 123 to 125
- **Cartesian** coordinate system: *x* and *y* are the X- and Y-axes.
- **Polar** coordinate system: *x* and *y* are theta and radius.
- **Gragraphic** coordinate system: *x* and *y* are parallels and meridians.
Copy link
Member

Choose a reason for hiding this comment

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

I am not really familiar with this parameter, and maybe I am wrong or confused by myself. I can follow the cases for "Cartesian" and "Polar", but for "Geographic" I am wondering if there is something mixed up. Looking at the image at the beginning of this PR (I reduced the example and added a red point at the starting point): For A="x", it's first in north-south direction (varying latitude, constant longitude, i.e. meridian) and then in east-west direction (varying longitude, constant latitude, i.e. parallel); and for A="y" it's the other way around. At the moment it looks to me a bit like that (only for the Geographic case) for the "x" and "y" arguments, people defined the x-axis as latitude (vertical) and the y-axis as longitude (horizontal), which is the other way around as the input data points are given. Comparing the images for the "Geographic" case with the images for the "Cartesian" case, I would expect the images for the "Geographic" case to be switched:

plot_A_geographic

import pygmt

fig = pygmt.Figure()
values = ["x", "y"]

for arg in values:
    fig.basemap(region=[0, 10, 1, 10], projection="X6c", frame=[f"WSen+t-A={arg}"])
    fig.plot(x=[3, 8], y=[2, 7], pen="1p", straight_line=arg)
    fig.plot(x=3, y=2, style="c0.2c", fill="red")
    fig.shift_origin(xshift="w+1")

fig.shift_origin(xshift="-2w-2", yshift="h+2")

for arg in values:
    fig.basemap(region=[0, 360, -90, 90], projection="H6c", frame=[f"WSen+t-A={arg}"])
    fig.plot(x=[90, 200], y=[-50, 50], pen="1p", straight_line=arg)
    fig.plot(x=90, y=-50, style="c0.2c", fill="red")
    fig.shift_origin(xshift="w+1")

fig.show()

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. It looks more like an upstream bug. Need time to verify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's confirmed to be an upstream bug at GenericMappingTools/gmt#8645 and has been fixed in GenericMappingTools/gmt#8648.

Unfortunately, there is no way to determine the type (Cartesian, polar or geographic) of the current coordinate system via public GMT APIs, so we can't have a fix on the PyGMT side. Instead, I have added a note for the bug 9aa8f97

@seisman seisman modified the milestones: 0.14.0, 0.15.0 Dec 31, 2024
@seisman seisman marked this pull request as draft December 31, 2024 00:29
@seisman seisman removed the needs review This PR has higher priority and needs review. label Dec 31, 2024
@seisman seisman removed this from the 0.15.0 milestone Dec 31, 2024
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
@seisman seisman removed the help wanted Helping hands are appreciated label Jan 2, 2025
@seisman seisman marked this pull request as ready for review January 2, 2025 11:09
@seisman seisman added this to the 0.15.0 milestone Jan 2, 2025
@seisman seisman added the needs review This PR has higher priority and needs review. label Jan 3, 2025
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. final review call This PR requires final review and approval from a second reviewer labels Jan 6, 2025
@seisman seisman merged commit e0f8e84 into main Jan 6, 2025
23 checks passed
@seisman seisman deleted the plot/straight_line branch January 6, 2025 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants