Skip to content

If called from external with -Vi or lower, print command string #4131

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 7 commits into from
Sep 30, 2020

Conversation

PaulWessel
Copy link
Member

For simpler debugging from external calls, we echo out the module call string when -Vi is used. Please give this a test, @seisman, to see if it works for PyGMT.

For simpler debugging from external calls, we echo out the module call string when -Vi is used.
@PaulWessel PaulWessel added bug Something isn't working backport 6.1 Backport this PR to 6.1 branch labels Aug 31, 2020
@PaulWessel PaulWessel requested a review from seisman August 31, 2020 03:19
@PaulWessel PaulWessel self-assigned this Aug 31, 2020
@PaulWessel PaulWessel requested a review from weiji14 August 31, 2020 03:24
@PaulWessel
Copy link
Member Author

Yes, they kind of get buried among low-level statements. Does it make sense for PyGMT to add some sort of debug-flag when you import, of is there no other place than passing a flag on the module call lines.

@joa-quim
Copy link
Member

joa-quim commented Aug 31, 2020

I don't need this from Julia because it's already implemented, though it wouldn't harm, but it should NOT go into -Vi. Current attempt doesn't work either

julia> plot(rand(5,2), V=:i, par=(GMT_VERBOSE=:information,), Vd=1)
GMT [INFORMATION]: GMT_Call_Command string: -C @GMTAPI@-S-I-D-M-T-N-000000 ->@GMTAPI@-S-O-D-D-T-N-000001
gmtinfo [INFORMATION]: Processing input table data
gmtinfo [INFORMATION]: Reading Data Table from Input memory location via matrix
gmtinfo [INFORMATION]: Writing Data Table to memory reference supplied by pointer
        psxy  -JX12c/8c -Baf -BWSen --GMT_VERBOSE=information -R0.04/0.68/0.06/0.96 -Vi -P -K > C:\TMP\GMTjl_tmp.ps
psxy [INFORMATION]: Processing input table data
psxy [INFORMATION]: Auto-frame interval for x-axis (item 0): a0.2f0.1
psxy [INFORMATION]: Auto-frame interval for y-axis (item 0): a0.2f0.1
psxy [INFORMATION]: Map scale is 5.33333e-05 km per cm or 1:5.33333.
psxy [INFORMATION]: Duplicating data table from user matrix location
psxy [INFORMATION]: Plotting segment 0

Note that the inline argument --GMT_VERBOSE=information doesn't work. I had to previously call gmtset GMT_VERBOSE i. Another issue is that the Julia plot command calls gmtinfo when no -R is provided, so what we see in the GMT_Call_Command string: -C @GMTAPI@-S-I-... is the response to that command, but then nothing is printed when psxy is called.

Probably the best place to implement this is in GMT_Create_Options(), when

if (n_args_in == 0) {	/* Check if a single command line, if so break into tokens */

although here it doesn't know the module name, and cannot ever know it from MEX & Julia because we call GMT_Create_Options() directly.

@PaulWessel
Copy link
Member Author

Do we hold onto this in master and remove the backport for now, @seisman?

@seisman seisman removed the backport 6.1 Backport this PR to 6.1 branch label Sep 2, 2020
@seisman
Copy link
Member

seisman commented Sep 2, 2020

Yes. "backport" removed.

@PaulWessel
Copy link
Member Author

Some comments on the comments:

  1. Yes, as mentioned one must need to do gmt set GMT_HISTORY information since --GMT_HISTORY=i is processed too late.
  2. Plotting the comment without the module name is clearly not helpful, so I think that rules out GMT_Create_Options.
  3. I forgot to add the module name before the options, now fixed.

Since this is to help debug, not for regular use by mere mortals, I do not think it is too much hardship to add a gmt set call to plant that setting before the main gmt commands. See if this minor update is good enough for government work.

Note: This PR still does not address @weiji14 point that these lines are not so visible given all the other stuff that is printed, which is the same as @seisman point of adding a higher level special verbosity to just kick in these messages.

@seisman
Copy link
Member

seisman commented Sep 21, 2020

Tried this Python script:

#!/usr/bin/env python
import pygmt

pygmt.config(GMT_VERBOSE='i')

fig = pygmt.Figure()
fig.basemap(region='g', projection='H15c', frame=['xaf', 'yaf', '+t"Global Map"'])
fig.coast(shorelines='1/0.5p')
fig.savefig("map.pdf")

And I get the full GMT commands by running the following command:

$ python test.py 2>&1 | grep GMT_Call_Command | awk -F': ' '{print "gmt", $3}'
gmt figure 71485df0ed3b4d4983d7a2d6bc2a5445 -
gmt figure 71485df0ed3b4d4983d7a2d6bc2a5445 -
gmt basemap -Bxaf -Byaf -B+t"Global Map" -JH15c -Rg
gmt figure 71485df0ed3b4d4983d7a2d6bc2a5445 -
gmt coast -W1/0.5p
gmt figure 71485df0ed3b4d4983d7a2d6bc2a5445 -
gmt psconvert -A -Fmap -Qg2 -Qt2 -Tf
gmt end
gmt figure 71485df0ed3b4d4983d7a2d6bc2a5445

Seems not bad for developers.

@weiji14 What do you think?

@seisman seisman removed the bug Something isn't working label Sep 21, 2020
@weiji14
Copy link
Member

weiji14 commented Sep 21, 2020

Looks much better! Probably not as ideal for users submitting bug reports (especially those on non-UNIX systems), but it should be enough for developers wanting to reproduce the commands. 👍 for me.

@PaulWessel
Copy link
Member Author

Any concerns for Julia, @joa-quim ?

@seisman
Copy link
Member

seisman commented Sep 21, 2020

I'm wondering if we should move the messages to -Vd instead, as they're only useful for debugging purposes.

@weiji14
Copy link
Member

weiji14 commented Sep 21, 2020

I'm wondering if we should move the messages to -Vd instead, as they're only useful for debugging purposes.

I suppose it should be -Vd. With the grep command, it should be easy enough to filter out the noisy debug messages (which was the original reason -Vi was used).

@joa-quim
Copy link
Member

Well, I mentioned above some quirks from Julia, but nothing serious because I won't rely on this (which would partially fail for the reason referred).

The best, but worky, solution would be to have a function that takes the linked list of options and build an options string. The reverse of one that I don't remember the name.

@seisman
Copy link
Member

seisman commented Sep 29, 2020

Ping @PaulWessel to finalize this PR.

@PaulWessel
Copy link
Member Author

The best, but worky, solution would be to have a function that takes the linked list of options and build an options string. The reverse of one that I don't remember the name.

Do you mean GMT_Create_Cmd ? We have all of those in the API.

@PaulWessel
Copy link
Member Author

To finalize, do you mean moving this to -Vd then, @seisman?

@seisman
Copy link
Member

seisman commented Sep 29, 2020

To finalize, do you mean moving this to -Vd then, @seisman?

Yes to me, but I don't fully understand what @joa-quim said.

@joa-quim
Copy link
Member

We have a function, GMT_Create_Options(), that converts a string with the options to a linked list. What I meant is that the ideal would be to have a function to do the reverse. Then the passed options would be shown as a single string.

@PaulWessel
Copy link
Member Author

And why is GMT_Create_Cmd not that function?

@joa-quim
Copy link
Member

It looks it is, so the solution is to use it to rebuild the cmd

@PaulWessel
Copy link
Member Author

Which is what this PR does, no?

@joa-quim
Copy link
Member

OK, I shut up. When I tested there were some issues but would need to repeat to learn again.

@PaulWessel
Copy link
Member Author

OK @seisman I have moved it to -Vd and I hope this one is good to go.

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

It's good, except a typo.

Co-authored-by: Dongdong Tian <[email protected]>
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.

4 participants