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

Change 12a43badc70bf5f4ea77b14b5044250c5ac16a76 from May 2 broke Julia #5384

Closed
joa-quim opened this issue Jun 25, 2021 · 16 comments · Fixed by #5392
Closed

Change 12a43badc70bf5f4ea77b14b5044250c5ac16a76 from May 2 broke Julia #5384

joa-quim opened this issue Jun 25, 2021 · 16 comments · Fixed by #5392
Labels
bug Something isn't working

Comments

@joa-quim
Copy link
Member

This now crashes

julia> bar(rand(15), color=:rainbow, show=1, Vd=2)
"psxy  -JX14c/9.5c -Baf -BWSen -R0.4/15.6/0/0.94 -C -i0-1,1 -Sb0.8u+b0 -P -K > C:\\TMP\\GMTjl_tmp.ps"

Very likely it's because it can't handle the -i0-1,1 anymore. The crash is indeed caused when it tries to access a 3rd column in data that has only 2 columns

@joa-quim joa-quim added the bug Something isn't working label Jun 25, 2021
@PaulWessel
Copy link
Member

OK, so a couple of things:

  1. We clearly need to improve how testing is done so that when we make any bug fixes on the API that there is either automatically or manually a run of the Julia and PyGMT tests (and of course ideally also GMT/MEX tests). Otherwise, time goes by and it is harder to get back in and rediscover what was learned. Maybe we should add a new label that can be used to flags API-related changes?
  2. Remind me if you are passing this as a matrix, sets of vectors, or a GMT dataset via reference.

Thoughts on this, @seisman and @meghanrjones?

@joa-quim
Copy link
Member Author

The detection problem here was that I didn't have this test in my test suites, or only tested that the GMT syntax is well generated.

The culprit was this in change in gmt_api.c/gmtapi_pick_in_col_number()

        col_pos = GMT->current.io.col[GMT_IN][col].col; /* Which data column to pick */
to
        col_pos = GMT->current.io.col[GMT_IN][col].order; /* Which data column to pick */

@seisman
Copy link
Member

seisman commented Jun 25, 2021

  1. We clearly need to improve how testing is done so that when we make any bug fixes on the API that there is either automatically or manually a run of the Julia and PyGMT tests (and of course ideally also GMT/MEX tests).

PyGMT runs its full tests with GMT master branch nightly, so any GMT API problems should be detected by PyGMT tests (if there are covered) in a few days.

The commit reverted in #5385 comes from PR #5289.

@PaulWessel
Copy link
Member

Please check if the revert (which was one line only) negatively affects the pyGMT examples.

@seisman
Copy link
Member

seisman commented Jun 25, 2021

Please check if the revert (which was one line only) negatively affects the pyGMT examples.

Yes, scripts in GenericMappingTools/pygmt#1313 now break.

@PaulWessel
Copy link
Member

Remind me if you are passing this as a matrix, sets of vectors, or a GMT dataset via reference.

@joa-quim let me know please.

@joa-quim
Copy link
Member Author

joa-quim commented Jun 25, 2021

I am passing a GMTdataset. It works if I only pass the matrix.

You can reproduce it in Matlab. This crashes Matlab (if using order instead of col)

D = gmt('wrapseg', {[(1:15)' rand(15,1)]})
gmt('info -C -i0-1,1', D)

@joa-quim
Copy link
Member Author

But even Matrices have problems. This should plot bars with error-bars and what is passed in is 5x3 matrix

men_means, men_std = (20, 35, 30, 35, 27), (2, 3, 4, 1, 2);
women_means, women_std = (25, 32, 34, 20, 25), (3, 5, 2, 3, 3);
x = collect(1:length(men_means));
width = 0.35;

bar(x.-width/2, collect(men_means), width=width, color=:lightblue,
    limits=(0.5,5.5,0,40), axis=:none, error_bars=(y=men_std,))
bar!(x.+width/2, collect(women_means), width=width, color=:brown,
    xaxis=(custom=(pos=1:5, label=[:G1 :G2 :G3 :G4 :G5]),), yaxis=(annot=5,label=:Scores),
    frame=(title="Scores by group and gender", axes=:WSrt), error_bars=(y=women_std,), savefig="genders.png")

The generated command is

cmd = " -R0.5/5.5/0/40 -JX14c/9.5c -Ey -C -i0-1,1,2-2 -Sb0.35u+b0"

No crash but 0-error bars

genders

@PaulWessel
Copy link
Member

I think I will make a test*.c example to do some more testing on -i and passing various things in.

@PaulWessel
Copy link
Member

And @seisman, I think PyGMT uses mostly GMT_VECTOR when passing in datasets and GMT_MATRIX when passing in grids, or do you mix it up?

@seisman
Copy link
Member

seisman commented Jun 25, 2021

And @seisman, I think PyGMT uses mostly GMT_VECTOR when passing in datasets and GMT_MATRIX when passing in grids, or do you mix it up?

I think PyGMT doesn't use GMT_VECTOR and GMT_MATRIX together.

@PaulWessel
Copy link
Member

I meant across all PyGMT modules you do use both methods for passing data into GMT. not mixing, but one modules may use VECTOR and another may use MATRIX.

@seisman
Copy link
Member

seisman commented Jun 25, 2021

one modules may use VECTOR and another may use MATRIX.

Yes.

@seisman
Copy link
Member

seisman commented Jun 25, 2021

one modules may use VECTOR and another may use MATRIX.

Yes.

Maybe I was wrong. For example, in PyGMT, the contour function can either pass vectors of x, y, z by calling GMT_Put_Vectors or pass a matrix of (x,y,z) by calling GMT_Put_Matrix.

@PaulWessel
Copy link
Member

Can you check this again - should work fine after the fix. if so please close.

@joa-quim
Copy link
Member Author

It's fine ... and closed.

@maxrjones maxrjones linked a pull request Aug 17, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants