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

Wrap coupe #3817

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Wrap coupe #3817

wants to merge 15 commits into from

Conversation

jhtong33
Copy link
Contributor

@jhtong33 jhtong33 commented Feb 21, 2025

Description of proposed changes

Adding coupe module to plot the plot cross-sections of focal mechanisms, following #2019 (comment) and #3551.
Test examples followed the existing scripts (description in test_coupe.py)

Some things I want to confirm:

Fixes #2019 and #3676

Preview: https://pygmt-dev--3817.org.readthedocs.build/en/3817/api/generated/pygmt.Figure.coupe.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.

Slash Commands

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

  • /format: automatically format and lint the code

@jhtong33
Copy link
Contributor Author

/format

@jhtong33 jhtong33 marked this pull request as draft February 21, 2025 15:17
@seisman seisman added the feature Brand new feature label Feb 23, 2025
Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

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

The whole code contains a lot of trailing white spaces. Please remove all of them.

**kwargs
):
r"""
Plot focal mechanisms in a cross section.
Copy link
Member

Choose a reason for hiding this comment

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

Most of the docstrings are the same as the one in Figure.meca. I'd prefer to remove all the docstrings for the following reasons:

  1. To avoid maintaining duplicated docstrings, maybe we can just add a link to Figure.meca and let users read Figre.meca docstrings first. If we decide to do this, we can remove these docstrings permanently.
  2. If we still want the full docstrings, we can still remove these docstrings temporarily, so that we can focus on the codes, rather than the docstrings in the initial reviews.

Copy link
Member

Choose a reason for hiding this comment

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

Would suggest to go with option 1. Another possibility would be to create a single docstring file and use it in both methods. That would avoid maintaining duplicated docstrings.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @michaelgrund! Maybe we can add these parameters to the decorators file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with @michaelgrund! Maybe we can add these parameters to the decorators file?

Also agree. Do I need to help with this part, or can someone else add these parameters to the decorators file (open new PR)?

Copy link
Member

Choose a reason for hiding this comment

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

I started to place the common docstrings into the decorators file (#3825). However I'm unsure how to handle the initial description which is also the same for meca/coupe. Afaik we do not have a similar case where we use the same description in two methods and thus this needs also to be added in the decorator.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add these parameters to the decorators file?

I don't think this is a good option, mainly because: (1) these are not real common docstrings. They're just shared by two or a few functions; (2) it will make the COMMON_DOCSTRING dictionary too big to maintain; (3) if we keep adding more "non-common" docstrings into COMMON_DOCSTRINGS in the future, we will likely have name conflicts and may make mistakes in using the placeholders.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @seisman regarding the size of the COMMON_DOCSTRING dictionary. That's why I initially just copied one parameter docstring in the POC PR. Any other suggestions how we could handle this without writing duplicate code?

Copy link
Member

Choose a reason for hiding this comment

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

Better to open a separate issue to discuss the possible solutions, so that this PR can focus on wrapping coupe.

Copy link
Member

@yvonnefroehlich yvonnefroehlich left a comment

Choose a reason for hiding this comment

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

A general aspect: Sofar, this PR is coming from @jhtong33's fork.
As she is a member of the PyGMT contributor team I believe she can create branches directly from the PyGMT repo. I feel this could make reviewing / testing this PR easier 🙂.

@seisman
Copy link
Member

seisman commented Feb 27, 2025

Actually, I'm thinking if it's possible to wrap the coupe module in Figure.meca. I.e., we may have a parameter like profile(True or False). If profile=True, then call coupe to plot beachballs on profiles, otherwise call meca to plot beachballs on maps. In this way, many codes can be reused. The idea may work or not depending on how many common options/features the two modules share, and we need take time to investigate it.

@jhtong33
Copy link
Contributor Author

Actually, I'm thinking if it's possible to wrap the coupe module in Figure.meca. I.e., we may have a parameter like profile(True or False). If profile=True, then call coupe to plot beachballs on profiles, otherwise call meca to plot beachballs on maps. In this way, many codes can be reused. The idea may work or not depending on how many common options/features the two modules share, and we need take time to investigate it.

The coupe module basically has two additional important parameters compared to meca. I’m wondering that users still need to specify the start and end longitude/latitude when profile=True in meca, similar to how it works in project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap coupe
4 participants