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.shift_origin: Improve docstrings, add inline examples and add type hints #2879

Merged
merged 23 commits into from
Jan 14, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Dec 14, 2023

pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
@seisman seisman added documentation Improvements or additions to documentation needs review This PR has higher priority and needs review. typing Type hints and static type checking labels Dec 15, 2023
@seisman seisman added this to the 0.11.0 milestone Dec 15, 2023
@seisman
Copy link
Member Author

seisman commented Dec 20, 2023

Ping @GenericMappingTools/pygmt-maintainers

Since we're working on the Figure.shift_origin function in this PR, I'd like to hear your thoughts about whether we should use xshift=5 or xshift="5c" in our gallery examples.

As mentioned in the updated docstrings:

Optionally, append the length unit (c for centimeters, i for inches, or p for points) to the shifts. Default unit if not explicitly given is c, but can be changed to other units via :gmt-term:PROJ_LENGTH_UNIT.

So, by default xshift=5 is the same as xshift="5c", but the default unit c can be changed by PROJ_LENGTH_UNIT or by setting set (UNITS "US") when building GMT source codes.

Personally, I prefer to xshift=5 for its simplicity. It's noteworthy to know that GMT recommends "always append the desired unit explicitly" (https://docs.generic-mapping-tools.org/dev/reference/features.html#dimension-units), but I feel we don't have to follow the recommendation in PyGMT.

Here are gallery examples that use xshift/yshift.

$ grep 'xshift=' examples/**/*.py
examples/gallery/3d_plots/scatter3d.py:fig.shift_origin(xshift=3.1)
examples/gallery/histograms/blockm.py:fig.shift_origin(xshift="w+5c")
examples/gallery/histograms/scatter_and_histograms.py:fig.shift_origin(yshift="-10.25c", xshift="10.25c")
examples/gallery/images/grdclip.py:fig.shift_origin(xshift="w+0.5c")
examples/gallery/images/grdgradient.py:fig.shift_origin(xshift="12.5c")
examples/gallery/lines/envelope.py:fig.shift_origin(xshift="11c")
examples/gallery/lines/envelope.py:fig.shift_origin(xshift="11c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="8c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="8c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="-16c", yshift="-7c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="8c", yshift="1.3c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="8c")
examples/tutorials/advanced/cartesian_histograms.py:fig.shift_origin(xshift="12c")
examples/tutorials/advanced/cartesian_histograms.py:fig.shift_origin(xshift="12c")
examples/tutorials/advanced/cartesian_histograms.py:fig.shift_origin(xshift="11c")
examples/tutorials/advanced/cartesian_histograms.py:fig.shift_origin(xshift="11c")
examples/tutorials/basics/coastlines.py:    fig.shift_origin(xshift="5c")

$ grep 'yshift=' examples/**/*.py
examples/gallery/histograms/scatter_and_histograms.py:fig.shift_origin(yshift="10.25c")
examples/gallery/histograms/scatter_and_histograms.py:fig.shift_origin(yshift="-10.25c", xshift="10.25c")
examples/gallery/images/cross_section.py:fig.shift_origin(yshift="12.5c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="-16c", yshift="-7c")
examples/projections/nongeo/polar.py:fig.shift_origin(xshift="8c", yshift="1.3c")
examples/tutorials/advanced/configuration.py:fig.shift_origin(yshift="-10c")
examples/tutorials/advanced/subplots.py:fig.shift_origin(yshift="h+1c")

Copy link

codspeed-hq bot commented Jan 2, 2024

CodSpeed Performance Report

Merging #2879 will not alter performance

⚠️ No base runs were found

Falling back to comparing docs/shift_origin (1148c88) with main (e0593d2)

Summary

✅ 64 untouched benchmarks

@weiji14
Copy link
Member

weiji14 commented Jan 2, 2024

So, by default xshift=5 is the same as xshift="5c", but the default unit c can be changed by PROJ_LENGTH_UNIT or by setting set (UNITS "US") when building GMT source codes.

Personally, I prefer to xshift=5 for its simplicity. It's noteworthy to know that GMT recommends "always append the desired unit explicitly" (https://docs.generic-mapping-tools.org/dev/reference/features.html#dimension-units), but I feel we don't have to follow the recommendation in PyGMT.

I'd prefer to explicitly use 5c so that people copying and pasing the code can reproduce the same figure even with a different PROJ_LENGTH_UNIT. We had some PRs that were strongly in favour of SI Units before (e.g. #719, #795).

@seisman
Copy link
Member Author

seisman commented Jan 2, 2024

So, by default xshift=5 is the same as xshift="5c", but the default unit c can be changed by PROJ_LENGTH_UNIT or by setting set (UNITS "US") when building GMT source codes.
Personally, I prefer to xshift=5 for its simplicity. It's noteworthy to know that GMT recommends "always append the desired unit explicitly" (https://docs.generic-mapping-tools.org/dev/reference/features.html#dimension-units), but I feel we don't have to follow the recommendation in PyGMT.

I'd prefer to explicitly use 5c so that people copying and pasing the code can reproduce the same figure even with a different PROJ_LENGTH_UNIT. We had some PRs that were strongly in favour of SI Units before (e.g. #719, #795).

OK. Updated one instance in 7ece697.

@yvonnefroehlich
Copy link
Member

Only passing the value is nice because of shortness and data type. But I feel for the examples, it is better to keep the unit because:

  • It's directly clear to the user which unit is used.
  • It's clear to the user that a specific unit can be specified.
  • If the user has changed the default value, the figure looks different from the one we have in the documentation. Maybe this is confusing, especially if the user is not familiar with setting the GMT defaults.
  • I agree with @weiji14's SI unit argument.

@seisman seisman requested a review from a team January 5, 2024 07:59
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
Co-authored-by: Yvonne Fröhlich <[email protected]>
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
Co-authored-by: Yvonne Fröhlich <[email protected]>
pygmt/src/shift_origin.py Outdated Show resolved Hide resolved
@seisman seisman requested a review from a team January 12, 2024 08:03
@seisman seisman merged commit 0374786 into main Jan 14, 2024
9 of 17 checks passed
@seisman seisman deleted the docs/shift_origin branch January 14, 2024 08:41
@seisman seisman removed the needs review This PR has higher priority and needs review. label Jan 14, 2024
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 typing Type hints and static type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants