Skip to content

Improve option checks and docs for grdgradient #6327

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

Merged
merged 6 commits into from
Feb 15, 2022
Merged

Conversation

maxrjones
Copy link
Member

Description of proposed changes

This PR fixes a docs mixup in which the direction (-D) and magnitude (-S) were switched.

@maxrjones
Copy link
Member Author

I'm opening this as a PR to have a discussion related to this section - the docs say that grdgradient computes the directional derivative or the magnitude/direction of the vector gradient. But there doesn't seem to be anything in the code that makes these options (-A versus -D|-S) mutually exclusive (e.g., GenericMappingTools/pygmt#1679). Is this intentional?

@maxrjones
Copy link
Member Author

I'm opening this as a PR to have a discussion related to this section - the docs say that grdgradient computes the directional derivative or the magnitude/direction of the vector gradient. But there doesn't seem to be anything in the code that makes these options (-A versus -D|-S) mutually exclusive (e.g., GenericMappingTools/pygmt#1679). Is this intentional?

Sorry, I forgot to ping @PaulWessel on this comment for feedback.

@PaulWessel
Copy link
Member

I think -A and -D are mutually exclusive since both write to the -G output grid. So you cannot get both the directional derivatives in the chosen direction (via -A) and output gradient directions by asking for -D as well. Likewise, -S can only be used with -D to save the magnitude to the separate file. So I think these changes are needed:

  1. Parse needs to give error if both -A and -D are set
  2. Parse needs to give error if -S is set but not -D
  3. Documentation for -D should start with "Instead of finding the gradient in a given direction (-A), find ..."

Note that if -S is given and -G is not then no gradient direction grid is written.

@maxrjones
Copy link
Member Author

I think -A and -D are mutually exclusive since both write to the -G output grid. So you cannot get both the directional derivatives in the chosen direction (via -A) and output gradient directions by asking for -D as well. Likewise, -S can only be used with -D to save the magnitude to the separate file. So I think these changes are needed:

1. Parse needs to give error if both **-A** and **-D** are set

2. Parse needs to give error if **-S** is set but not **-D**

3. Documentation for **-D** should start with "Instead of finding the gradient in a given direction (-A), find ..."

Note that if -S is given and -G is not then no gradient direction grid is written.

Sounds good, thanks for the input. I'll submit a PR with these changes after I finish drafting up a conference abstract.

@PaulWessel
Copy link
Member

Also, while at it. I think the equations with words look bad. I suggest you change this to use variable names. Just introduce variables o (offset) and \sigma (sigma) [which you already have], amplitude (a), and then it will look more like real equations.

@maxrjones
Copy link
Member Author

Sounds good. I'm going to convert this to a WIP and add the changes here rather than opening a different PR, since this already contains the relevant discussion.

@maxrjones maxrjones changed the title Fix mix-up in grdgradient docs WIP: Improve option checks and docs for grdgradient Feb 9, 2022
@maxrjones maxrjones self-assigned this Feb 9, 2022
@maxrjones maxrjones requested a review from PaulWessel February 14, 2022 23:49
Copy link
Member

@PaulWessel PaulWessel left a comment

Choose a reason for hiding this comment

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

There is a :math: run-in in the -A section that is not set as math.
-Nt last equation, is the parenthesis around a needed?
Otherwise looks great!

@maxrjones maxrjones changed the title WIP: Improve option checks and docs for grdgradient Improve option checks and docs for grdgradient Feb 15, 2022
@maxrjones maxrjones merged commit 5908c7f into master Feb 15, 2022
@maxrjones maxrjones deleted the docs-grdgradient branch February 15, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants