Skip to content

gmtinfo -i from externals is screwing on Windows #5311

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

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

gmtinfo -i from externals is screwing on Windows #5311

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

Comments

@joa-quim
Copy link
Member

joa-quim commented Jun 7, 2021

Probably the source of the PyGMT test failure on Win

julia> gmtinfo(rand(5,3), i="0-2,2", C=true)
Vector{GMT.GMTdataset} with 1 segments
First segment DATA
1×8 Matrix{Float64}:
 0.147981  0.882137  0.366351  0.884463  0.0  2.25401e-315  0.0  2.25401e-315

but it works well on Linux

julia> gmtinfo(rand(5,3), i="0-2,2", C=true)
Array{GMT.GMTdataset,1} with 1 segments
First segment DATA
1×8 Array{Float64,2}:
 0.0540404  0.662781  0.173727  0.691856  0.0268022  0.900573  0.0268022  0.900573

It works well on Win if instead of an array we pass in a file name or if we pass a simpler (and redundant) -i0-2 option

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

In matlab on mac it works:

a = gmt ('info', '-C -i0-2,2', rand(5,3))

a =

struct with fields:

   data: [0.1270 0.9134 0.0975 0.9649 0.1576 0.9706 0.1576 0.9706]

If this is a win gmt api issue the it should also fail for you in matlab on Windows, right?

@joa-quim
Copy link
Member Author

joa-quim commented Jun 7, 2021

Yes, it fails on Matlab too. Seems to return uninitialized memory for the last 4 elements.

@PaulWessel
Copy link
Member

OK, maybe you can step through then and look for any clues since I cannot make it fail.

@PaulWessel
Copy link
Member

I should mention that we very recently made edits to gmt_api.c related to reading both rec-by-rec (gmt info I think) and tables from virtual memory given as vectors, and related to -i. Are you passing these as a matrix or set of vectors?

@PaulWessel
Copy link
Member

Can you please verify that you are building with latest GMT (6.2.0 or master)? I cannot see anything obvious in the code section and it works in mex for me. These assignments take places in gmtinfo.c directly into GMT->current.io.curr_rec which is BUFSIZE long, so if things are wrong it is hard to guess where that happens without you stepping through for a clue.

@joa-quim
Copy link
Member Author

joa-quim commented Jun 8, 2021

I better be using 6.2.0 otherwise the Win installer ... 😄

I'll try to find it.

@PaulWessel
Copy link
Member

I looked in the GMT_Get_Record that takes you into the read record from matrix section but could not see anything wrong...hence we need a hint.

@joa-quim
Copy link
Member Author

joa-quim commented Jun 9, 2021

What I have found so far is that:

  1. We have Chinese binaries inside GMT
  2. With gmtinfo(rand(5,3), i="0-2", C=true), the GMT->current.io.col[GMT_IN][col].order is like the first fig
    image
  3. But with gmtinfo(rand(5,3), i="0-2,2", C=true it is

image

@joa-quim
Copy link
Member Author

joa-quim commented Jun 9, 2021

Must be the f qsort. The reordering happens at line 9138 of gmt_init.c

qsort (GMT->current.io.col[GMT_IN], k, sizeof (struct GMT_COL_INFO), gmtinit_compare_cols);

Remember that we once had an issue with it and I even opened an issue in Microsoft that they refused saying the behavior was undefined by C standards so the shit they did was legal.

@PaulWessel
Copy link
Member

Sorry, was on a zoom. Looking now. I do vaguely remember qsort etc. Are you able to see what the order is before qsort? But would not this problem also affect command line usage since this is the common parsing of -i? I cannot see how a bug there would not show up equally in mex, Julia AND command line?

@PaulWessel
Copy link
Member

Is it this issue you mean: https://github.com/MicrosoftDocs/cpp-docs/issues/2303

@joa-quim
Copy link
Member Author

joa-quim commented Jun 9, 2021

Don't know if a repeated one but it's not the same because the other one was opened by me. And pre GH times.

The command line does the same thing but they are not used later. Instead the col_pos is increased monotonically in gmt_io#3660, whilst the externals read in gmt_api#9589 that now use the GMT->current.io.col[GMT_IN][k].order and the order is bad.

@PaulWessel
Copy link
Member

Wonder if you could build GMT with our compat/qsort instead. I think it means getting HAVE_QSORT_R_GLIBC set to false so the code is included. As a check, you could edit compat/qsort.? and change

#ifndef HAVE_QSORT_R_GLIBC

to

#ifdef WIN32

and that should do it!

@joa-quim
Copy link
Member Author

joa-quim commented Jun 9, 2021

Nope. That file has qsort_r but not qsort and the number of input arguments is different.
The bug report seems similar to my old one, as well as the (unqualifiable) attitude of considering that behavior normal.

The problem is definitely the qsort call. Commenting that line produces the expected result.

@PaulWessel
Copy link
Member

From macos man page on qsort_r:

The algorithms implemented by qsort(), qsort_r(), and heapsort() are not stable; that is, if two members compare as equal, their order in the sorted array is undefined. The mergesort() algorithm is stable.

Maybe you could replace that qsort with mergesort (same args) and see what happens? Speed here is irrelevant since we are sorting tiny arrays, so stability is better.

@joa-quim
Copy link
Member Author

joa-quim commented Jun 9, 2021

Yep, mergesort works.

THIS QSORT(S) THING IS ABSOLUTELY INSANE.

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.

2 participants