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

The -i for memory datasets via matrix used the wrong parameter #5289

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

PaulWessel
Copy link
Member

Instead of the column order it echoed back the column loop variable. Addresses GenericMappingTools/pygmt#1313, I hope.

Instead of the column order it echoed back the column loop variable.
@PaulWessel PaulWessel requested review from seisman and maxrjones June 1, 2021 14:52
@PaulWessel PaulWessel self-assigned this Jun 1, 2021
@seisman
Copy link
Member

seisman commented Jun 1, 2021

This PR fixes the initial issue, but makes other tests failing. Here is an failing example using this branch (datafile: points.txt):

import pygmt
import numpy as np


datafile = "points.txt"

# file
fig = pygmt.Figure()
fig.plot3d(
    data=datafile,
    zscale=5,
    perspective=[225, 30],
    region=[10, 70, -5, 10, 0, 1],
    projection="X10c",
    style="c0.5c",
    cmap="rainbow",
    frame=["a", "za"],
    i=[0, 1, 2, 2],
)
fig.savefig("plot3d-file.png")

# array
data = np.loadtxt(datafile)
fig = pygmt.Figure()
fig.plot3d(
    data=data,
    zscale=5,
    perspective=[225, 30],
    region=[10, 70, -5, 10, 0, 1],
    projection="X10c",
    style="c0.5c",
    cmap="rainbow",
    frame=["a", "za"],
    i=[0, 1, 2, 2],
)
fig.savefig("plot3d-array.png")
plot3d-file plot3d-array
plot3d-file plot3d-array

@PaulWessel
Copy link
Member Author

I have Chair duty this morning so will be slow looking at this. But I now what the issue is: the gmtio_ascii_input has a loop to handle repeated columns while the column function does not. So there is some replication to do I think where it is being called (4+ times in gmt_api.c). I will see how far I can get.

@PaulWessel
Copy link
Member Author

PaulWessel commented Jun 1, 2021

Got this far but no longer. python/miniconda/pygmt not doing arm64 I guess?

pygmt.exceptions.GMTCLibNotFoundError: Error loading GMT shared library at '/Users/pwessel/UH/RESEARCH/CVSPROJECTS/GMTdev/gmt-dev/xbuild/src/Debug/libgmt.dylib'.
dlopen(/Users/pwessel/UH/RESEARCH/CVSPROJECTS/GMTdev/gmt-dev/xbuild/src/Debug/libgmt.dylib, 6): no suitable image found.  Did find:
	/Users/pwessel/UH/RESEARCH/CVSPROJECTS/GMTdev/gmt-dev/xbuild/src/Debug/libgmt.dylib: mach-o, but wrong architecture

I am on the laptop this morning so not done this recently.

@PaulWessel
Copy link
Member Author

I committed my test changes for this case (see lines 9576-9589 in gmt_api.c. Perhaps you can try this to see - it may have bugs since I am unable to go through it, but it is similar to what gmtio_ascii_input does when it loops over the tokens.

Should it miraculously work as is then there are other places in gmt_api.c that need a similar upgrade.

@maxrjones
Copy link
Member

I am still getting the same result as Dongdong posted above with the lastest commit.

@PaulWessel
Copy link
Member Author

OK, well then if you have time to debug this, compare that new code with what happens for data files (gmt_io.c lines 3638-3676, but especially lines 3663 onwards [also note 3639-3645. Otherwise late afternoon for me when I get back to the safety of my home office.

@maxrjones
Copy link
Member

OK, well then if you have time to debug this, compare that new code with what happens for data files (gmt_io.c lines 3638-3676, but especially lines 3663 onwards [also note 3639-3645. Otherwise late afternoon for me when I get back to the safety of my home office.

OK, I will take a look.

src/gmt_api.c Outdated Show resolved Hide resolved
Co-authored-by: Meghan Jones <[email protected]>
@PaulWessel
Copy link
Member Author

OK, great - have a check to see if it fixes all of them.

@seisman
Copy link
Member

seisman commented Jun 1, 2021

There are still some issues:

  1. The original contour script in The "incols" (-i) parameter doesn't works for vector input correctly pygmt#1313 now crashes for me.
  2. The plot3d example in The -i for memory datasets via matrix used the wrong parameter #5289 (comment) works
  3. Current PyGMT tests pass, except the tests in test_rose.py hang and maybe are waiting for inputs.

@maxrjones
Copy link
Member

The test_rose.py test hangs because of the change on L1319. The values read into GMT->current.io.curr_rec are outlandishly large (~10e18) and there is a section in psrose.c that increments by 1 down to 360. @PaulWessel, can you take a look at gmt_api.c ~L9642 to check whether any other updates are needed to correspond to the change on L1319:

for (col = 0; col < API->current_get_n_columns; col++) {	/* We know the number of columns from registration */
			col_pos = gmtapi_pick_in_col_number (GMT, (unsigned int)col);
			API->current_get_V_val[col] (&(V->data[col_pos]), S->rec, &(GMT->current.io.curr_rec[col]));
		}

@maxrjones
Copy link
Member

I am not able to reproduce the crash in the original contour script. Do you get any error messages with the crash?

@seisman
Copy link
Member

seisman commented Jun 1, 2021

I am not able to reproduce the crash in the original contour script. Do you get any error messages with the crash?

I used incols but forgot to switch back to the GenericMappingTools/pygmt#1303 branch. Now the contour example passes for me. So the only problem is the "rose" tests.

@PaulWessel
Copy link
Member Author

PaulWessel commented Jun 2, 2021

Just arrived home, have 50 minutes until a zoom. So api_get_record_vector need similar fix, no? I can take a first crack at it since I know @meghanrjones is probably busy with her REU student.

@PaulWessel
Copy link
Member Author

Just checking, which of the rose figs fail? The ones with the incols args? Both of them? Or OK to just try one?

@PaulWessel
Copy link
Member Author

And given my lack of Python chops, would be best if you could copy/paste into this issue a simple failing script.

@seisman
Copy link
Member

seisman commented Jun 2, 2021

And given my lack of Python chops, would be best if you could copy/paste into this issue a simple failing script.

Please try this one:

from pygmt import Figure
from pygmt.datasets import load_fractures_compilation


data = load_fractures_compilation()
fig = Figure()
fig.rose(
    data=data,
    region=[0, 1, 0, 360],
    sector=15,
    diameter="5.5c",
    color="blue",
    frame=["x0.2g0.2", "y30g30", "+glightgray"],
    incols=[1, 0],
    pen="1p",
    norm="",
    scale=0.4,
)
fig.show()

@PaulWessel
Copy link
Member Author

Thanks, so I can see it is hanging. Ctrl-C does not seem to kill the fig.rose call in the terminal - is this normal? Looks like I have to kill that terminal window or the python process?

@seisman
Copy link
Member

seisman commented Jun 2, 2021

Ctrl-C does not seem to kill the fig.rose call in the terminal - is this normal? Looks like I have to kill that terminal window or the python process?

It also bothers me a lot. I don't have a solution yet.

@PaulWessel
Copy link
Member Author

Thanks, not a real problem but killing the terminal was the quickest.
So from debug it seems the input was set up via GMT_IS_VECTOR and that vector 0 is GMT_DOUBLE and vector 1 is GMT_LONG. I can tell record one is 272.954, 0. So the vector is passing azimuth, length and since rose is expecting length, azimuth the -i0,1 is passed. All this is fine I think. It looks like the mixup is using the wrong decode function. I will play with that. It is a good test with mixing the types.

@PaulWessel
Copy link
Member Author

OK, fixed that I think - hopefully no other side-effects; let me know.

@seisman
Copy link
Member

seisman commented Jun 2, 2021

The rose tests no longer wait for inputs, but the images are wrong.

The above test uses the @fractures_06.txt file, so I expect a rose diagram like https://docs.generic-mapping-tools.org/dev/gallery/ex06.html.

Expected Actual
image image

@PaulWessel
Copy link
Member Author

OK, and sorry I got confused by the numbers above. The 272.954 is actually the length and 0 is the azimuth, Given that the GMT_IS_VECTOR passes 272.95,0 it is passing length,azimuth which is the expected data format for gmt rose. The @fractures_06.txt file contains angle,azimuth, hence it needs -:. Adding -i1,0 in the present case would reverse that and give the plot you obtained. So I wonder how the GMT_IS_VECTOR structure is assembled since it seems it already has the columns switched. How was it made?

@seisman
Copy link
Member

seisman commented Jun 2, 2021

OK, and sorry I got confused by the numbers above. The 272.954 is actually the length and 0 is the azimuth, Given that the GMT_IS_VECTOR passes 272.95,0 it is passing length,azimuth which is the expected data format for gmt rose. The @fractures_06.txt file contains angle,azimuth, hence it needs -:. Adding -i1,0 in the present case would reverse that and give the plot you obtained. So I wonder how the GMT_IS_VECTOR structure is assembled since it seems it already has the columns switched. How was it made?

I just realized that although the @fractures_06.txt file contains "azimuth, length", the load_fractures_compilation function used in the above example actually returns "length, azimuth". In this case, -i is not needed, or -i0,1 should be used, rather than -i1,0. It means the PyGMT tests are actually wrong. Makes sense, right?

@seisman
Copy link
Member

seisman commented Jun 2, 2021

After correcting the column order returned by the load_fractures_compilation function, all PyGMT tests pass and the original contour test also passes.

@PaulWessel
Copy link
Member Author

Puh, great!

@PaulWessel PaulWessel changed the title WIP The -i for memory datasets via matrix used the wrong parameter The -i for memory datasets via matrix used the wrong parameter Jun 2, 2021
@PaulWessel PaulWessel merged commit 12a43ba into master Jun 2, 2021
@PaulWessel PaulWessel deleted the bad-col-pick branch June 2, 2021 02:58
@michaelgrund
Copy link
Member

6.1.1 6.2.0
ex06_611 ex06_620

By the way, the appearance of the grid lines in rose figures using 6.2.0 (gridlines in background) slightly changed compared to 6.1.1. (in foreground). Is there an (undocumented) option available to change that @PaulWessel @meghanrjones ?

@maxrjones
Copy link
Member

By the way, the appearance of the grid lines in rose figures using 6.2.0 (gridlines in background) slightly changed compared to 6.1.1. (in foreground). Is there an (undocumented) option available to change that @PaulWessel @meghanrjones ?

I think this is probably a consequence of #4274, fixing #4265. @PaulWessel, can you confirm that the above plot order for rose in 6.2.0 is the intended order?

@michaelgrund The way around this is to do a separate basemap command after rose.

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