Skip to content

Conversation

@PaulWessel
Copy link
Member

See #5153 for background. This PR seeks to implement that scheme. The FORMAT_GEO_OUT documentation now explains that the F and G can be used with FORMAT_GEO_MAP as either prefix or suffix. Seems to work but need @KristofKoch to give it a better test. Note that it does not work with FORMAT_GEO_OUT since the result would be data that GMT would only recognise as text (since it starts with a letter). If there is a need to produce data output like

W23:55 S19:44
E16:05 N05:15

then we would need to recognise the above as valid data ascii input when reading it. Not sure if we want to go that road yet.

Example:

gmt basemap -R-1/1/-1/1 -JM15c -Baf -png map --FORMAT_GEO_MAP=Gddd.mm

map

We should probably add a test. I can imagine a 2x2 subplot with this example using F and G as suffix or prefix which gives 4 cases.

@PaulWessel PaulWessel added the enhancement Improving an existing feature label Jan 14, 2023
@PaulWessel PaulWessel added this to the 6.5.0 milestone Jan 14, 2023
@PaulWessel PaulWessel self-assigned this Jan 14, 2023
@PaulWessel
Copy link
Member Author

I am thinking perhaps we either ban outright or warn that if FORMAT_GEO_OUT uses G (which adds a space) then the output is not compatible with GMT data format anymore and cannot be recognised properly:

echo -13 14 | gmt convert --FORMAT_GEO_OUT=ddd:mmG -fg
13:00 W	14:00 N

Output is geographic but because of G has that extra space between numbers and suffices. If we then try to read that back in:

echo -13 14 | gmt convert --FORMAT_GEO_OUT=ddd:mmG -fg | gmt convert -fg
13	W	14:00 N

We cannot get past the columns with just a W or E and the whole thing becomes just trailing text as is. I see no use for this. What do you think: Prevent it or just warn when parsing FORMAT_GEO_OUT? I guess the latter is the most flexible since you may want to format a data set for a completely other non-GMT use.

@KristofKoch
Copy link
Contributor

Hi Paul, leading [F|G] worked great in my limited testing so far. I noticed one thing:

When using --FORMAT_GEO_MAP=Fddd:mmF (which is clearly not formatted correctly with the double occurrence F at the beginning and at the end) only the last occurrence is honoured. Maybe a warning would be a nice idea? Just as a heads up in the sense of "Hey, your formatting can't work this way! Figure it out, in the meantime we just use the last occurrence of [F|G].".

@KristofKoch
Copy link
Contributor

What do you think: Prevent it or just warn when parsing FORMAT_GEO_OUT? I guess the latter is the most flexible since you may want to format a data set for a completely other non-GMT use.

I agree with you and would just warn. While I mostly use floating point for the internal stuff there might be non-GMT use cases where this could be desirable.

@PaulWessel
Copy link
Member Author

Thanks @KristofKoch. For now, I have added checks for multiple F, G or combinations and they get the general error message (there are too many error possibilities we check for to give individual messages). I now warn if you use trailing G in FORMAT_GEO_OUT since it adds that space. For now, since the output function does not yet handle pre-number hemisphere I have just added commented-out warning to remind me to eventually let GMT write out the bad format and open this message.

Copy link
Contributor

@KristofKoch KristofKoch left a comment

Choose a reason for hiding this comment

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

I like it. That error message gives me a nice starting point for investigating what I screwed up. Thank you, @PaulWessel!

@KristofKoch KristofKoch merged commit 22700b8 into master Jan 15, 2023
@KristofKoch KristofKoch deleted the pre-hemisphere branch January 15, 2023 16:30
@maxrjones maxrjones added the add-changelog Add PR to the changelog label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add-changelog Add PR to the changelog enhancement Improving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants