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

Unify potential supplement modules to use z positive up for relief #6818

Open
PaulWessel opened this issue Jun 23, 2022 · 5 comments
Open

Unify potential supplement modules to use z positive up for relief #6818

PaulWessel opened this issue Jun 23, 2022 · 5 comments
Assignees
Labels
feature request Request a new feature

Comments

@PaulWessel
Copy link
Member

Description of the desired feature

Except for grdredpol, all potential modules either accept a topographic grid or profile, and/or in some cases create a topographic grid or profile. Topographic input to modules computing flexure (gmtflexure, grdflexure, gravfft -T) all agree that the load surface has z positive up. However, the signs of other input and output data vary across the supplement which leads to frustration and confusion (I speak as a user now):

  1. Modules mostlly written by @joa-quim expects topographic reliefs to have z positive up (gmtgravmag3d, gravfft, grdgravmag3d) while modules mostly written by me expects the relief to be positive down (gmtflexure, grdflexure, talwani2d, talwani3d, with gravprisms unsure). Some of this is inherited from various algorithms.
  2. Some modules have an option to flip the positive direction of z: -D in grdgravmag3d and -A in gravprisms, talwani2d and talwani3d, but the rest do not.
  3. The documentation for several modules are unclear or do not state what the positive z-direction is.
  4. Modules that take options like water depth or compensation depth at times expect a z-level (so sign is important) and at other times expect a distance (so is a positive quantity), but the documentation mixes this up in unfortunate ways.

Because this supplement is only used by geophysicists doing grav, mag, flexure, any changes we do have limited impact. Here are suggestions for changes to make this supplement self-consistent.

  1. We declare there are bugs and fix all modules so that the z-axis for topography is positive up by default. This will introduce a breaking change in some of the modules listed in (1), but bug fixes usually are.
  2. We remove those options but leave their effect as is under the hood for backwards compatibility. Or, we also remove the parsing and effect so that users actually find out about this issue.
  3. We improve the documentation by having a common statement include block at the top of each module that clarifies the coordinate system used.
  4. Instead of options to flip signs, the documentation can simply state that if your grid is positive down you can flip the sign on input by appending +s-1, and for data tables you can use -i2+s-1 (or whatever column z is). These are common modifiers available to any module in GMT so having to add -A -D and waste limited option letters is bad style anyway.
  5. ???

I think it is a nice goal to have the entire supplement agree on where positive z goes, and because of the limited impact I am willing to declare bugs to fix this mess. However, happy to hear comments from @GenericMappingTools/core.

@PaulWessel PaulWessel added the feature request Request a new feature label Jun 23, 2022
@PaulWessel PaulWessel added this to the 6.5.0 milestone Jun 23, 2022
@PaulWessel PaulWessel self-assigned this Jun 23, 2022
@joa-quim
Copy link
Member

I think we should go 1. My main reason to advocate this are the wrappers. If working from one of those environments it will be common to have the bathymetry/topography grid already in memory. Then to use different modules that use different z sign convention one would be obliged to, eventually back and forth, multiply the grid by -1 and not only tell the GMT reading function to do it when reading the data.

Then all of these modules should have that initial warning about the vertical system used and point users to the +s modifiers to change data sign.

@PaulWessel
Copy link
Member Author

PaulWessel commented Jun 23, 2022

What is your thought regarding action 2? Since taking action 1 changes the default direction for some modules, having -A to restore what it was seems a bit silly since the meaning of -A now has changed too.

@joa-quim
Copy link
Member

joa-quim commented Jun 23, 2022

Remove them but let them active. Thanks to Ubuntu we'll have many old versions around for many years and users may consult manuals of older versions and in fact use newer ones or pass scripts to others that are more up to date.

Edit: ohs, sorry I replied when you had only up to the question marque.
Ok, remove code too. -s will always be there.

@PaulWessel
Copy link
Member Author

OK, since this supplement is just you and me I think there is a plan for action. I will go through each module slowly (especially as I am using many of them in research) and try to avoid giant PRs covering everything.

@PaulWessel
Copy link
Member Author

I should say that as I am starting to look at mine, e.g. gmtflexure, it actually reports flexure as z positive up, so perhaps the damage is less wide-spread than I first reported...

PaulWessel added a commit that referenced this issue Jun 23, 2022
See #6818 for background.  This PR improves the discussion of water depth (related to density contrasts) and reference depth (-Z, now specified as a distance).  Also added an include _rst file that explains the geometry.  This file will be included in other modules as I revise them.
@seisman seisman modified the milestones: 6.4.0, 6.5.0 Jun 25, 2022
@PaulWessel PaulWessel removed this from the 6.5.0 milestone Jul 19, 2023
PaulWessel added a commit that referenced this issue Dec 9, 2023
* Clarify z is positive up and eliminate -A in gravprisms

See #6818 for background.  This PR improves the discussion of water depth (related to density contrasts) and reference depth (-Z, now specified as a distance).  Also added an include _rst file that explains the geometry.  This file will be included in other modules as I revise them.

* finalize removal of -A and update test

* Update gravprisms.rst

* Update docs also

* Update gravprisms.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request a new feature
Projects
None yet
Development

No branches or pull requests

3 participants