Skip to content

Update common options in THIS_MODULE_OPTIONS and docs#5759

Merged
maxrjones merged 33 commits intomasterfrom
consistent-common
Sep 17, 2021
Merged

Update common options in THIS_MODULE_OPTIONS and docs#5759
maxrjones merged 33 commits intomasterfrom
consistent-common

Conversation

@maxrjones
Copy link
Member

@maxrjones maxrjones commented Sep 14, 2021

Description of proposed changes

This PR updates erroneous contents in the THIS_MODULE_OPTIONS macro and ensures consistency between the macro and documentation, motivated by @PaulWessel's observation that some modules inappropriately have the -s common option without offering table output.

Also includes some minor docs fixes.

Fixes #5653
Fixes #5670

Reminders

  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Describe changes to function behavior and arguments in a comment below the function declaration.
  • If adding new functionality, add a detailed description to the documentation and/or an example.

@maxrjones
Copy link
Member Author

@PaulWessel, could you please clarify common option usage for a couple modules?

  1. blockmean, blockmode, blockmedian include -g in the options macro, which is undocumented. My interpretation based on the docs and some tests is that this option does not provide any value for these modules. Could you confirm that this option can be removed?
  2. Should gmtbinstats include -s? Currently -s is included and documented, but I cannot find a way to include empty bins in the output which would suggest that it should be excluded (similar to blockm* modules).

Also, is there a historical reason why -s is only available for output records, rather than input and/or output like -q, -d, -h, and -f?

@PaulWessel
Copy link
Member

@PaulWessel, could you please clarify common option usage for a couple modules?

  1. blockmean, blockmode, blockmedian include -g in the options macro, which is undocumented. My interpretation based on the docs and some tests is that this option does not provide any value for these modules. Could you confirm that this option can be removed?

Yes, I cannot think of any reason -g is useful in this context.

  1. Should gmtbinstats include -s? Currently -s is included and documented, but I cannot find a way to include empty bins in the output which would suggest that it should be excluded (similar to blockm* modules).

I agree with that too.

Also, is there a historical reason why -s is only available for output records, rather than input and/or output like -q, -d, -h, and -f?

Yes. It originated as a special -S option in grd2xyz to exclude output if a node was NaN. Then it migrated to a global common -s option in GMT 5 since there were other modules where not outputting a record if z = NaN made sense. There was no clear need for it to apply on input: If input records has NaNs GMT handles that very differently. NaNs are allowed except for in x-y coordinates I think. There is the GMT defaults IO_NAN_RECORDS [pass] that deals with how NaNs should be considered if found in input.

@maxrjones maxrjones self-assigned this Sep 15, 2021
@maxrjones maxrjones changed the title WIP: Check common options in THIS_MODULE_OPTIONS and docs Update common options in THIS_MODULE_OPTIONS and docs Sep 17, 2021
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.

Great! Lots of little minor bugs fixed along the way as well I see.

@maxrjones maxrjones merged commit 62b6fd8 into master Sep 17, 2021
@maxrjones maxrjones deleted the consistent-common branch September 17, 2021 21:13
@maxrjones maxrjones added the documentation Improve documentation label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improve documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix minor documentation errors Add -s common option to ReST documentation for various modules

2 participants