search

GenericMappingTools / gmt

The Generic Mapping Tools
https://www.generic-mapping-tools.org
Other
843 stars 352 forks source link

An inset bug #4757

Closed seisman closed 3 years ago

seisman commented 3 years ago

Full script that generated the error

The issue was originally found in https://github.com/GenericMappingTools/pygmt/pull/788

gmt begin map png
  gmt coast -Ba -Gbrown -RMG+r2 -Slightblue -Wthin
  gmt inset begin -DjTL+w3.5c+o0.2c -F+pgreen -M0
    gmt coast -EMG+gred -Ggray -JG47/-20/?c -Rg -Swhite
  gmt inset end
gmt end show

Full error message

coast [WARNING]: 1.37795i not a valid number and may not be decoded properly.
plot [WARNING]: 1.37795i not a valid number and may not be decoded properly.
inset [WARNING]: 1.37795i not a valid number and may not be decoded properly.
Actual outcome Expected outcome
image image

The red Madagascar is not incorrectly placed.

System information

PaulWessel commented 3 years ago

Seems you have a hanging "c" after -JG47/-20/? which is not expected. There is no reason to append a unit to something that will be computed. So i would suggest we do this:

  1. Give error if more than ? is found, or we can be nice and just ignore anything beyond ?. Right now we replace ? with 1.37795i and you get 1.37795ic.
  2. Try to expedite the plan to eliminate the ? altogether since it is problematic for some shells, so that -JG47/-20/ is all that should be given?

For plan 2, I think it will be just as easy to find either a lone trailing / -in -J or look for the few cases where the scale or width is not the last argument (-JP I think) so that we look for //, meaning a missing argument, no?

seisman commented 3 years ago

Seems you have a hanging "c" after -JG47/-20/? which is not expected. There is no reason to append a unit to something that will be computed.

Yes, it makes sense.

For plan 2, I think it will be just as easy to find either a lone trailing / -in -J or look for the few cases where the scale or width is not the last argument (-JP I think) so that we look for //, meaning a missing argument, no?

We discussed this before in https://github.com/GenericMappingTools/gmt/issues/3916. I'm OK with eliminating ?, but I don't like it, because I feel that 47/-20/ or 47/-20//60 is not as readable as 47/-20/? or 47/-20/?/60. Especially users may only type a single / even though the documentation uses //.

PaulWessel commented 3 years ago

Perhaps we can make the ? optional. We have to be backwards compatible anyway. But the csh ? problem is real so need to be addressed.

PaulWessel commented 3 years ago

So perhaps close this as not really a bug but feature?

seisman commented 3 years ago

Perhaps we can make the ? optional.

Yes, this would be better!

seisman commented 3 years ago

Closing as not a bug.

joa-quim commented 3 years ago

Perhaps we can make the ? optional.

Don't remember how but I'm using the? in Julia generated commands.

PaulWessel commented 3 years ago

Yep, not getting rid of it but making that optional is the goal.