Skip to content

Make the -J question-mark for subplots and insets optional #4758

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 10, 2021

Conversation

PaulWessel
Copy link
Member

Description of proposed changes

Since the perfect map scale or width for plots placed in subplot panels or insets to fit that space exactly are unknown to the user, we introduced in GMT 6 the syntax that the user should supply "?" where normally a scale or width should go. GMT looks for these and replaces them with the computed quantities. This works fine, except we have since learned that some UNIX shells do not want to play nice (see #3916). Then what? We cannot let GMT csh scripts not work.

This PR implements an alternative scheme while still honoring the initial solution, and is thus backwards compatible. In the new scheme, the ? is not provided, yielding a missing argument. Most map projections have the size as the final argument, while Cartesian and polar can be more complicated. My implementation works this way: It looks for the -J argument to determine if it is of the form where the scale/dimension is missing and there is no ?, and if so inserts the missing question-mark(s). It then passes the modified argument to our existing parser.

With this PR, the following lists have valid -J arguments in insets and subplot panels and on the right is the converted argument:

-JA-45/60/   ->  -JA-45/60/?
-JA-45/60   ->  -JA-45/60/?
-JX-/  -> -JX-?/?
-JM   -> -JM?
-JP+a45  -> -JP?+a45
-JH10 -> -JH10/?
-JH10/  -> -JH10/?

etc. Running the tests with this PR yielded 7 failures:

	 98 - doc/scripts/GMT_inset.sh (Failed)
	222 - doc/examples/ex44/ex44.sh (Failed)
	313 - test/gmtbinstats/nonweighted.sh (Failed)
	314 - test/gmtbinstats/nonweightedrect.sh (Failed)
	315 - test/gmtbinstats/weighted.sh (Failed)
	316 - test/gmtbinstats/weightedrect.sh (Failed)
	558 - test/modern/inset.sh (Failed)

Upon inspection, these were all due to these scripts actually spelling out the desired dimension of the inset or panel plot instead of relying on the magic (some where composed before I figured out the ? mechanism). Since it is allowed to actually give a manual size, I added a check that if a -J ends with a dimension indicator (c|i|p(, then we leave it alone. With that change all tests passes.

However, the reason this has a WIP is so we can discuss what to do. Insets and subplot panels are still a relatively new thing in GMT. It is likely that most users supply the ? in the -J option, and both the Julia and Python wrappers do that as well. However, we would currently fail if someone gave a specific projection and size without using a trailing unit, e.g. -JA-45/60/13. This of course goes against the magic of ? and also our recommendation of supplying dimension units in scripts to avoid side-effects when posting or sharing scripts - but users do this anyway. The only way to really avoid falling into that trap is to add much more checking so that we know exactly how many slashes each projection should have, and if there is an argument after the last one then do nothing. This is complicated by the fact that many projections have optional arguments, such as central meridian, and then it may not be possible to tell for sure. I do not want to attempt this coding.

I think we have these two options that both will simplify GMT syntax and fix #3916:

  1. We insist the final slash be present, e.g., -JA-45/60/. This is then foolproof.
  2. We take the chance and allow even the slash to be missing, with the caveat listed above.

In both cases we need to update the documentation. We should also fix the above 7 script to use the auto magic and not provide bad examples in the cookbook.

Happy to entertain votes on this issue. I think option (1) is the conservative, safe choice, but open to comments.

Given the issues with csh discussed in #3916, this PR makes the questionmark optional.  It even allows for a lack of final slash.
@PaulWessel PaulWessel added this to the 6.2.0 milestone Feb 6, 2021
@PaulWessel PaulWessel requested a review from a team February 6, 2021 07:21
@PaulWessel PaulWessel self-assigned this Feb 6, 2021
@PaulWessel
Copy link
Member Author

Having slept on this, I think option 1 is the only feasible solution. That way we are 100% backwards compatible and can allow the ? to be there or not. Will wrap that up tomorrow.

@PaulWessel
Copy link
Member Author

PaulWessel commented Feb 7, 2021

To be sure I get this right I looked at all the -J forms we have. Remember that the current system using ? in places for scales works (except under csh perhaps). So if I detect a ? in the -J string I do nothing. If there is no ? character then I need to examine if the user specified a scale/width or if they left it off for us to add the ?.

Here they are classified based on syntax:

1. Projections that always ends in /scale (or /width):

-Ja|A<lon0>/<lat0>[/<horizon>]/
-Jb|B<lon0>/<lat0>/<lat1>/<lat2>/
-Jc|C<lon0>/<lat0>/
-Jcyl_stere|Cyl_stere/[<lon0>/[<lat0>/]]
-Jd|D<lon0>/<lat0>/<lat1>/<lat2>/
-Je|E<lon0>/<lat0>[/<horizon>]/
-Jf|F<lon0>/<lat0>[/<horizon>]/
-Jg|G<lon0>/<lat0>/
-Jg|G<lon0>/<lat0>/<altitude>/<azimuth>/<tilt>/<twist>/<Width>/<Height>/
-Jl|L<lon0>/<lat0>/<lat1>/<lat2>/
-Jo|O[a|A]<lon0>/<lat0>/<azimuth>/
-Jo|O[b|B]<lon0>/<lat0>/<lon1>/<lat1>/
-Jo|Oc|C<lon0>/<lat0>/<lonp>/<latp>/
-Js|S<lon0>/<lat0>[/<horizon>]/
-Jt|T<lon0>/[<lat0>/]

For these we check if string ends with / and if so we append ?.

2. Projections that may not have a slash (e.g., -Jm):

-Jh|H[<lon0>/]
-Ji|I[<lon0>/]
-Jj|J[<lon0>/]
-Jkf|Kf[<lon0>/]
-Jk|K[s][<lon0>/]
-Jm|M[<lon0>/[<lat0>/]]
-Jn|N[<lon0>/]
-Jpoly|Poly/[<lon0>/[<lat0>/]]
-Jq|Q[<lon0>/[<lat0>/]]
-Jr|R[<lon0>/]
-Jv|V[<lon0>/]
-Jw|W[<lon0>/]
-Jy|Y[<lon0>/[<lat0>/]]
-Ju|U[<zone>/]

If they do use the optional arg then there is a slash and they move up to the simple category 1. Otherwise, we check if there is a number where the scale should go, and if not we append ?.

  1. Special Cartesian cases
-Jx|X[/]
-Jp|P[+a][+f[e|p|<radius>]][+r<offset>][+t<origin>][+z[p|<radius>]]

Because -Jx may use a slash to separate two different scales we examine this projection separately, so that -Jx/ becomes -Jx?/? and -Jx becomes -Jx?. We also look for the d,l,p stuff for degree, log, power. For the polar projection we similarly check if there is a scale or not after -Jp so that -Jp+a becomes -Jp?+a, etc.

4. proj syntax (-Jproj|EPSG:n)

I do not think these are (yet) useable in plots but assuming they are/will be we simply need to watch for the two extensions +width=size and +scale=1:xxxx and if there is no argument after the = we will place the ? there.

Does @GenericMappingTools/core see any non-unique cases here or do I have them all covered?

@joa-quim
Copy link
Member

joa-quim commented Feb 8, 2021

... all of this because there are 20 persons in the world that still use csh and don't want to bash my.sh 😈

@PaulWessel
Copy link
Member Author

Well, as long as Dave Sandwell and his group are doing csh we kinda have to make it work. But this also shortens your typing (you save one ? per inset or panel.

Require a trailing / if at least one another argument, or otherwise have no numbers after the projection code.  Adjusted the demonstration inset script and docs. added complement script that tests -JX without ?.
@PaulWessel PaulWessel changed the title WIP Make the -J question-mark for subplots and insets optional Make the -J question-mark for subplots and insets optional Feb 8, 2021
@PaulWessel
Copy link
Member Author

I have completed this PR and stayed with the conservative option (1) above and followed the three projection cases to finish the implementation. Updated the docs accordinly and doc/scripts. Left most test scripts using ? as they were so they are exercised, but added on new without ? to test the many -JX perturbations (e.g., -J-/d -> -J-?/?d). Pinging @seisman, @joa-quim and @meghanrjones for final test and approval.

@PaulWessel
Copy link
Member Author

I would like to move on and have this PR approved. Just to remind you, in testing I found that our documentation plot of insets (doc/script/GMT_inset.sh) looked like this:

gmt begin GMT_inset
	gmt coast -R110E/170E/44S/9S -JM6i -B -BWSne -Wfaint -N2/1p  -EAU+gbisque -Gbrown -Sazure1 -Da -Xc --FORMAT_GEO_MAP=dddF
	gmt inset begin -DjTR+w1.5i+o0.15i -F+gwhite+p1p+s -M0.05i
	gmt coast -Rg -JG120/30S/1.4i -Da -Gbrown -A5000 -Bg -Wfaint -EAU+gbisque
	gmt inset end
gmt end show

Despite setting up the inset size and margin, the coast command in the inset still set the actual size of 1.4 inches. Per our GMT 6.0 documentation this should have been using the questionmark:

gmt begin GMT_inset
	gmt coast -R110E/170E/44S/9S -JM6i -B -BWSne -Wfaint -N2/1p  -EAU+gbisque -Gbrown -Sazure1 -Da -Xc --FORMAT_GEO_MAP=dddF
	gmt inset begin -DjTR+w1.5i+o0.15i -F+gwhite+p1p+s -M0.05i
	gmt coast -Rg -JG120/30S/? -Da -Gbrown -A5000 -Bg -Wfaint -EAU+gbisque
	gmt inset end
gmt end show

What this PR does is to relax the requirement of the question mark (which fails in csh), so that this is the preferred way:

gmt begin GMT_inset
	gmt coast -R110E/170E/44S/9S -JM6i -B -BWSne -Wfaint -N2/1p  -EAU+gbisque -Gbrown -Sazure1 -Da -Xc --FORMAT_GEO_MAP=dddF
	gmt inset begin -DjTR+w1.5i+o0.15i -F+gwhite+p1p+s -M0.05i
	gmt coast -Rg -JG120/30S/ -Da -Gbrown -A5000 -Bg -Wfaint -EAU+gbisque
	gmt inset end
gmt end show

All of these varients (but importantly the last two) give the same plot and is thus backwards compatible. The implementation actually replaces -JG120/30S/ with -JG120/30S/? and then calls the regular ?-parser. Please have a check if you'd like and then consider giving your approval(s).

@PaulWessel PaulWessel merged commit 2789263 into master Feb 10, 2021
@PaulWessel PaulWessel deleted the questionmark-optional branch February 10, 2021 01:46
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.

Trouble with some shells like csh
3 participants