Skip to content

Rewrite how -i works internally for external sources#5392

Merged
PaulWessel merged 8 commits intomasterfrom
test-columns-api
Jun 27, 2021
Merged

Rewrite how -i works internally for external sources#5392
PaulWessel merged 8 commits intomasterfrom
test-columns-api

Conversation

@PaulWessel
Copy link
Member

Description of proposed changes

There were several conceptual issues with how we had implemented -i when reading from matrices and vectors (and datasets) from memory. This has now been fixed (I hope) and we are now using a procedure more like this:

for (col = 0; col < n_columns; col++) {
   col_pos_out = gmtlib_pick_in_col_number (GMT, col, &col_pos_in);
   output[col_pos_out] = gmt_M_convert_col (GMT->current.io.col[GMT_IN][col], input[col_pos_in]);
}

Thus, the col is just a dummy loop variable and the in/out-cols can be independent from that. col_pos_out covers the same values that col does, but not necessarily in any particular order, and col_pos_in may go outside that range and even have repeats. I added a new test program (testapi_columns.c) driven by test script apicolumns_icol.sh, which passes. Note: Only testing matrices and vectors here as testing datasets revealed some unrelated issues that will be dealt with later.

Keeping this at WIP to see if it fixes recent trouble related to external tables in PyGMT (@seisman) and GMT.jl (@joa-quim).

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

The previous broken cases now work well with this branch.

@PaulWessel PaulWessel changed the title WIP Rewrite how -i works internally for external sources Rewrite how -i works internally for external sources Jun 26, 2021
@PaulWessel
Copy link
Member Author

Thanks, taking of WIP. The old fix was a wack-a-mole fix. Fixed one thing and broke another. No way to fix both without this rewrite. Once tested with PyGMT and approved I will merge and then work on similar upgrades for a few other places in the API where this appears, and it may likely lead to improvements such as doing -o2,1,2,2 etc being allowed.

@joa-quim
Copy link
Member

Thanks. And at the end we'll need another release.

@seisman
Copy link
Member

seisman commented Jun 26, 2021

The original contour test now works, but there is still a failure for rose.

The load_fractures_compilation loads the @fractures_06.txt file and returns a DataArray with two columns, "length" and "azimuth". In the first figure, "length" and "azimuth" is passed, so the figure is correct. In the second figure, "azimuth" and "length" is passed, so incols=[1, 0] is used to swap the input columns, but the second figure is wrong:

import pygmt
from pygmt.datasets import load_fractures_compilation

data = load_fractures_compilation()

data1 = data[["length", "azimuth"]]
fig = pygmt.Figure()
fig.rose(
    data=data1,
    region=[0, 1, 0, 360],
    sector=10,
    diameter="10c",
    frame=["x0.2g0.2", "y30g30", "+glightgray"],
    color="red3",
    pen="1p",
    orientation=False,
    norm=True,
    vectors=True,
    no_scale=True,
    shift=False,
)
fig.savefig("rose1.png")

data2 = data[["azimuth", "length"]]
fig = pygmt.Figure()
fig.rose(
    data=data2,
    incols=[1, 0],
    region=[0, 1, 0, 360],
    sector=10,
    diameter="10c",
    frame=["x0.2g0.2", "y30g30", "+glightgray"],
    color="red3",
    pen="1p",
    orientation=False,
    norm=True,
    vectors=True,
    no_scale=True,
    shift=False,
)
fig.savefig("rose2.png")
First figure Second figure
image image

@PaulWessel
Copy link
Member Author

OK, will look at this later - have some errands that need to be done first...

@PaulWessel
Copy link
Member Author

Should be OK now, @seisman.

@seisman
Copy link
Member

seisman commented Jun 27, 2021

Now all PyGMT tests pass.

@maxrjones
Copy link
Member

@PaulWessel , you mentioned "Only testing matrices and vectors here as testing datasets revealed some unrelated issues that will be dealt with later". I'm debugging GenericMappingTools/pygmt#1440 and find that the col_pos used in:

gmt/src/gmt_api.c

Lines 3932 to 3940 in b33d6d8

for (col = 0; col < V_obj->n_columns; col++) {
if (GMT->common.i.select) /* -i has selected some columns */
col_pos = GMT->current.io.col[GMT_IN][col].col; /* Which data column to pick */
else if (GMT->current.setting.io_lonlat_toggle[GMT_IN] && col < GMT_Z) /* Worry about -: for lon,lat */
col_pos = 1 - col; /* Read lat/lon instead of lon/lat */
else
col_pos = col; /* Just goto that column */
D_obj->table[D_obj->n_tables]->segment[0]->data[col] = V_obj->data[col_pos].f8;
}

is incorrect. Do you know if this is related to the known issues for for gmtapi_import_dataset? If not, I can continue debugging but don't want to repeat efforts.

@PaulWessel
Copy link
Member Author

No, please continue, and let me know if you need help.

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 this pull request may close these issues.

Change 12a43badc70bf5f4ea77b14b5044250c5ac16a76 from May 2 broke Julia

4 participants