Skip to content

Update grdfill parameters for GMT 6.2.0#1283

Merged
weiji14 merged 16 commits intomasterfrom
grdfill-parameters
Jun 16, 2021
Merged

Update grdfill parameters for GMT 6.2.0#1283
weiji14 merged 16 commits intomasterfrom
grdfill-parameters

Conversation

@willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented May 22, 2021 •
edited by weiji14
Loading

This pull request adds additional parameters to the grdfill module.

Continuation of work on #1276

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 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 commands are:

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

@willschlitzer willschlitzer added enhancement Improving an existing feature skip-changelog Skip adding Pull Request to changelog labels May 22, 2021
@willschlitzer willschlitzer added this to the 0.4.0 milestone May 22, 2021
@willschlitzer willschlitzer self-assigned this May 22, 2021
seisman
seisman reviewed May 22, 2021
View reviewed changes
@willschlitzer @seisman
Update pygmt/src/grdfill.py
8a64273 
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@weiji14
Copy link
Member

weiji14 commented May 23, 2021 •
edited
Loading

Just surfacing a point made in #1276:

What is the preferred was for denoting required parameters? I think there should be some indication in the method docs that mode (-A) is required.

Perhaps just a one-line description in the docstring wll suffice?

Could you please add a line in the docstring that says mode is a required parameter? I'll also leave it up to you to decide whether a GMTInvalidInput error should be raised if mode is not provided (requires coding up a few lines).

Edit: Maybe hold on first, -A might not be a required parameter in grdfill, see GenericMappingTools/gmt#5247 (comment).

@willschlitzer
Copy link
Contributor Author

willschlitzer commented May 24, 2021 •
edited
Loading

Just surfacing a point made in #1276:

What is the preferred was for denoting required parameters? I think there should be some indication in the method docs that mode (-A) is required.

Perhaps just a one-line description in the docstring wll suffice?

Could you please add a line in the docstring that says mode is a required parameter? I'll also leave it up to you to decide whether a GMTInvalidInput error should be raised if mode is not provided (requires coding up a few lines).

Edit: Maybe hold on first, -A might not be a required parameter in grdfill, see GenericMappingTools/gmt#5247 (comment).

It looks like @PaulWessel is planning on making -A required. I'm not sure what the timeline is for that, so I added a test to check that grdfill works with only a grid argument.

@maxrjones
Copy link
Member

maxrjones commented May 24, 2021

Another update from that gmt grdfill PR: either -A or -L is required. The bug fix will be merged after 6.2.0rc2 and likely before 6.2.0.

I think the best solution for pygmt is to check that either -A or -L are provided and give a GMTInvalidInput error if not (similar to pygmt/src/coast.py L190-194 and the corresponding test in pygmt/tests/test_coast.py L36 - 42).

@willschlitzer
Copy link
Contributor Author

willschlitzer commented May 25, 2021

Another update from that gmt grdfill PR: either -A or -L is required. The bug fix will be merged after 6.2.0rc2 and likely before 6.2.0.

I think the best solution for pygmt is to check that either -A or -L are provided and give a GMTInvalidInput error if not (similar to pygmt/src/coast.py L190-194 and the corresponding test in pygmt/tests/test_coast.py L36 - 42).

I made A and L required arguments, but the error that is raised only specifies A being required, as I haven't wrapped L in this pull request. I couldn't figure out how to get around it not returning a grid object/file.

seisman
seisman reviewed May 26, 2021
View reviewed changes
@willschlitzer @seisman
Update pygmt/src/grdfill.py
e28e323 
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label May 28, 2021
core-man
core-man approved these changes May 29, 2021
View reviewed changes
Copy link
Member

@core-man core-man left a comment

Choose a reason for hiding this comment

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

Looks great~

@seisman
Copy link
Member

seisman commented May 29, 2021

@meghanrjones Could you please give this PR a review, since you know more about the changes on the GMT side.

@maxrjones
Copy link
Member

maxrjones commented May 29, 2021

Is the plan to add -L separately?

The s option for mode should eventually be added (it was implemented before 6.2.0rc2 - just was a documentation error that stated it was not yet an option). This could be a separate PR if needed.

@seisman
Copy link
Member

seisman commented May 30, 2021

Is the plan to add -L separately?

Option -L output some information to stdout, rather than modifying an input grid. I believe it needs more effort to think about how to return the -L output and can be done in a separate PR.

The s option for mode should eventually be added (it was implemented before 6.2.0rc2 - just was a documentation error that stated it was not yet an option). This could be a separate PR if needed.

I think this documentation issue is small and should be included into this PR.

@seisman
Copy link
Member

seisman commented Jun 3, 2021

Ping @willschlitzer to improve the description of -A option, that mode="s" is allowed (See https://docs.generic-mapping-tools.org/dev/grdfill.html).

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Jun 6, 2021

Ping @willschlitzer to improve the description of -A option, that mode="s" is allowed (See https://docs.generic-mapping-tools.org/dev/grdfill.html).

I'm planning on doing this after I get back to my home on June 14th.

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Jun 6, 2021
weiji14
weiji14 reviewed Jun 15, 2021
View reviewed changes
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.

Welcome back @willschlitzer! Hope you had some good time off. Just one review comment for now 😃

maxrjones
maxrjones approved these changes Jun 15, 2021
View reviewed changes
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

One tiny comment, otherwise looks great!

@maxrjones maxrjones mentioned this pull request Jun 15, 2021
Closed
27 tasks
@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Jun 15, 2021
@willschlitzer
Update pygmt/src/grdfill.py
206b6fc 
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
weiji14
weiji14 approved these changes Jun 15, 2021
View reviewed changes
@willschlitzer @weiji14
Update pygmt/tests/test_grdfill.py
bd82fda 
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@weiji14 weiji14 changed the title Update grdfill parameters Update grdfill parameters for GMT 6.2.0 Jun 16, 2021
@weiji14 weiji14 merged commit 623fe21 into master Jun 16, 2021
@weiji14 weiji14 deleted the grdfill-parameters branch June 16, 2021 21:48
@willschlitzer willschlitzer removed the final review call This PR requires final review and approval from a second reviewer label Jun 17, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
@willschlitzer @seisman @weiji14
Update grdfill parameters for GMT 6.2.0 (GenericMappingTools#1283)
7066eb4 
Add an additional `verbose` (-V) parameter to the `grdfill` module,
include the new mode='s' option, and set 'mode' and 'L' as required
parameters.

* fix top docstring
* make A/L required arguments
* add s option in docs for mode in grdfill

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seisman seisman seisman approved these changes

@maxrjones maxrjones maxrjones approved these changes

@michaelgrund michaelgrund michaelgrund approved these changes

@weiji14 weiji14 weiji14 approved these changes

@core-man core-man core-man approved these changes

Assignees

@willschlitzer willschlitzer

Labels

enhancement Improving an existing feature skip-changelog Skip adding Pull Request to changelog

Projects

None yet

Milestone

0.4.0

Development

Successfully merging this pull request may close these issues.

6 participants

@willschlitzer @weiji14 @maxrjones @seisman @michaelgrund @core-man