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

pygmt.surface: Add aliases for "C", "L", "M", and "T" #2321

Merged
merged 30 commits into from
Feb 7, 2023

Conversation

jhtong33
Copy link
Contributor

@jhtong33 jhtong33 commented Jan 11, 2023

Description of proposed changes
adding T=''tensor in surface.py

Fixes #2222

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

@welcome
Copy link

welcome bot commented Jan 11, 2023

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

  • If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
  • All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
  • No matter what, we are really grateful that you put in the effort to do this! 🎉

@yvonnefroehlich
Copy link
Member

Welcome 🙂 and great that you started working on this issue @JingHuiTong!

Please add the related issue number #2222 under Fixes # by editing your initial comment. Then this issue is automatically closed when this PR is merged.

Beside adding an alias for T to the alias list, we also need to add some documentation (docstrings) to explain the usage of this parameter. You may want to orientate yourself on the upstream GMT documentation at https://docs.generic-mapping-tools.org/dev/surface.html#t.

Parameters
----------
data : str or {table-like}
Pass in (x, y, z) or (longitude, latitude, elevation) values by
providing a file name to an ASCII data table, a 2D
{table-classes}.
x/y/z : 1d arrays
Arrays of x and y coordinates and values z of the data points.
{spacing}
{region}
outgrid : str
Optional. The file name for the output netcdf file with extension .nc
to store the grid in.

One tipp for further contributions, which are highly welcome 🚀: It is better to create a feature branch from the main branch of your personal fork, instead of directly pushing to the main branch.

@yvonnefroehlich yvonnefroehlich added this to the 0.9.0 milestone Jan 11, 2023
@yvonnefroehlich yvonnefroehlich added documentation Improvements or additions to documentation enhancement Improving an existing feature labels Jan 11, 2023
@yvonnefroehlich yvonnefroehlich changed the title alias surface.py T=tensor Figure.surface: Add an alias for "T" (tension factor) Jan 11, 2023
@yvonnefroehlich yvonnefroehlich changed the title Figure.surface: Add an alias for "T" (tension factor) Figure.surface: Add alias "tension" for "T" (tension factor) Jan 11, 2023
@seisman seisman removed the documentation Improvements or additions to documentation label Jan 12, 2023
@seisman seisman changed the title Figure.surface: Add alias "tension" for "T" (tension factor) pygmt.surface: Add alias "tension" for "T" (tension factor) Jan 13, 2023
@jhtong33
Copy link
Contributor Author

May I add more alias -L -M -C? (Inspire by #1660)

@weiji14
Copy link
Member

weiji14 commented Jan 13, 2023

May I add more alias -L -M -C? (Inspire by #1660)

Sure! But you'll need to document each one 😁

@yvonnefroehlich yvonnefroehlich changed the title pygmt.surface: Add alias "tension" for "T" (tension factor) pygmt.surface: Add alias for "T", "L", "M", and "C" Jan 14, 2023
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

To fix the 'Style Checks', you can either write /format as a comment to have the automated bot format things automatically on GitHub, or do it manually as I mentioned just now in the video call 😉 Let us know if you have any other questions!

pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Jan 15, 2023

Sorry, I meant /format in a GitHub comment, not git commit comment 😅

@jhtong33
Copy link
Contributor Author

Ha! I know it after trying.

@weiji14 weiji14 added the needs review This PR has higher priority and needs review. label Feb 6, 2023
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Hi @JingHuiTong, hope you had a nice holiday break! Just one last suggestion, otherwise everything looks good to me!

Will wait for one or two other reviewers to approve before we merge this in, just to double check 😉

pygmt/src/surface.py Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
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.

Looks good except the minor comment and the merge conflicts.

examples/gallery/images/grdgradient_shading.py Outdated Show resolved Hide resolved
@jhtong33
Copy link
Contributor Author

jhtong33 commented Feb 6, 2023

/format

pygmt/src/surface.py Outdated Show resolved Hide resolved
@weiji14 weiji14 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. labels Feb 7, 2023
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
pygmt/src/surface.py Outdated Show resolved Hide resolved
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Yvonne Fröhlich <[email protected]>
@weiji14 weiji14 enabled auto-merge (squash) February 7, 2023 09:16
@weiji14 weiji14 merged commit 59a2035 into GenericMappingTools:main Feb 7, 2023
@welcome
Copy link

welcome bot commented Feb 7, 2023

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@weiji14
Copy link
Member

weiji14 commented Feb 7, 2023

Woohoo, thanks again @JingHuiTong! Definitely let us know if you're interested in contributing more stuff, I'm sure you've got plenty of fresh ideas :D

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.

[pygmt.surface] tension_factor
6 participants