Skip to content

Commit

Permalink
Fix a bug when passing data to GMT in Session.open_virtual_file() (#490)
Browse files Browse the repository at this point in the history
External programs like PyGMT can pass dataset/momory to GMT. By default,
GMT can read, modify and free the momery, which sometimes can cause
crashes.

Issue #406 reports an example in which PyGMT crashes. The issue was reported
to the upstream (see GenericMappingTools/gmt#3515
and GenericMappingTools/gmt#3528). It turns out
to be a API user error (i.e., a PyGMT bug).

As per the explanation of Paul, external programs like PyGMT should
always use `GMT_IN|GMT_IS_REFERENCE` to tell GMT that the data is
read-only, so that GMT won't try to change and free the memory.

This PR makes the change from `GMT_IN` to `GMT_IN|GMT_IS_REFERENCE`
in the `Session.open_virtual_file()` function, updates a few docstrings,
and also adds the script in #406 as a test.
  • Loading branch information
seisman authored Jun 24, 2020
1 parent eaf81bb commit 516e799
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 5 deletions.
13 changes: 10 additions & 3 deletions pygmt/clib/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,9 @@ def open_virtual_file(self, family, geometry, direction, data):
direction : str
Either ``'GMT_IN'`` or ``'GMT_OUT'`` to indicate if passing data to
GMT or getting it out of GMT, respectively.
By default, GMT can modify the data you pass in. Add modifier
``'GMT_IS_REFERENCE'`` to tell GMT the data are read-only, or
``'GMT_IS_DUPLICATE'' to tell GMT to duplicate the data.
data : int
The ctypes void pointer to your GMT data structure.
Expand Down Expand Up @@ -950,7 +953,7 @@ def open_virtual_file(self, family, geometry, direction, data):
... lib.put_vector(dataset, column=0, vector=x)
... lib.put_vector(dataset, column=1, vector=y)
... # Add the dataset to a virtual file
... vfargs = (family, geometry, 'GMT_IN', dataset)
... vfargs = (family, geometry, 'GMT_IN|GMT_IS_REFERENCE', dataset)
... with lib.open_virtual_file(*vfargs) as vfile:
... # Send the output to a temp file so that we can read it
... with GMTTempFile() as ofile:
Expand Down Expand Up @@ -1086,7 +1089,9 @@ def virtualfile_from_vectors(self, *vectors):
for col, array in enumerate(arrays):
self.put_vector(dataset, column=col, vector=array)

with self.open_virtual_file(family, geometry, "GMT_IN", dataset) as vfile:
with self.open_virtual_file(
family, geometry, "GMT_IN|GMT_IS_REFERENCE", dataset
) as vfile:
yield vfile

@contextmanager
Expand Down Expand Up @@ -1167,7 +1172,9 @@ def virtualfile_from_matrix(self, matrix):

self.put_matrix(dataset, matrix)

with self.open_virtual_file(family, geometry, "GMT_IN", dataset) as vfile:
with self.open_virtual_file(
family, geometry, "GMT_IN|GMT_IS_REFERENCE", dataset
) as vfile:
yield vfile

@contextmanager
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 7 additions & 2 deletions pygmt/tests/test_clib.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ def test_virtual_file():
data = np.arange(shape[0] * shape[1], dtype=dtype).reshape(shape)
lib.put_matrix(dataset, matrix=data)
# Add the dataset to a virtual file and pass it along to gmt info
vfargs = (family, geometry, "GMT_IN", dataset)
vfargs = (family, geometry, "GMT_IN|GMT_IS_REFERENCE", dataset)
with lib.open_virtual_file(*vfargs) as vfile:
with GMTTempFile() as outfile:
lib.call_module("info", "{} ->{}".format(vfile, outfile.name))
Expand All @@ -491,7 +491,12 @@ def test_virtual_file_fails():
Check that opening and closing virtual files raises an exception for
non-zero return codes
"""
vfargs = ("GMT_IS_DATASET|GMT_VIA_MATRIX", "GMT_IS_POINT", "GMT_IN", None)
vfargs = (
"GMT_IS_DATASET|GMT_VIA_MATRIX",
"GMT_IS_POINT",
"GMT_IN|GMT_IS_REFERENCE",
None,
)

# Mock Open_VirtualFile to test the status check when entering the context.
# If the exception is raised, the code won't get to the closing of the
Expand Down
17 changes: 17 additions & 0 deletions pygmt/tests/test_plot.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,23 @@ def test_plot_vectors():
return fig


@pytest.mark.mpl_image_compare
def test_plot_lines_with_arrows():
"""Plot lines with arrows.
The test is slightly different from test_plot_vectors().
Here the vectors are plotted as lines, with arrows at the end.
The test also check if the API crashes.
See https://github.com/GenericMappingTools/pygmt/issues/406.
"""
fig = Figure()
fig.basemap(region=[-2, 2, -2, 2], frame=True)
fig.plot(x=[-1.0, -1.0], y=[-1.0, 1.0], pen="1p,black+ve0.2c")
fig.plot(x=[1.0, 1.0], y=[-1.0, 1.0], pen="1p,black+ve0.2c")
return fig


@pytest.mark.mpl_image_compare
def test_plot_scalar_xy():
"Plot symbols given scalar x, y coordinates"
Expand Down

0 comments on commit 516e799

Please sign in to comment.