GenericMappingTools / gmt

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

gmtinfo -i from externals is screwing on Windows #5311

Closed joa-quim closed 3 years ago

joa-quim commented 3 years ago

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

PaulWessel commented 3 years ago

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 commented 3 years ago

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

PaulWessel commented 3 years ago

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

PaulWessel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

I'll try to find it.

PaulWessel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

joa-quim commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Yep, mergesort works.

THIS QSORT(S) THING IS ABSOLUTELY INSANE.