GenericMappingTools / gmt

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

GMT_Get_Default should always return a value for API_IMAGE_LAYOUT #4892

Closed seisman closed 3 years ago

seisman commented 3 years ago

https://github.com/GenericMappingTools/gmt/blob/be0e6d591b8074a6cf9aef9a0567c931264bd5bf/src/gmt_api.c#L13037-L13040

The GMT_Get_Default() function only returns the value of API_IMAGE_LAYOUT if GMT is linked with GDAL. However, for external programs like PyGMT, it's difficult to know if a GMT library is linked with GDAL or not. So calling GMT_Get_Default() may report an error.

It would be better if GMT_Get_Default can report an invalid value for API_IMAGE_LAYOUT if it's not linked.

joa-quim commented 3 years ago

While that is true and easy to fix (it can return an empty string), the absence of GDAL linkage together with the attempt to use images would have caused an error before. Or, said other way, why trying to access to API_IMAGE_LAYOUT if you don't have any image?

seisman commented 3 years ago

Currently, PyGMT doesn't use API_IMAGE_LAYOUT at all, but it tries to get the value of API_IMAGE_LAYOUT and save it for further use when loading the library. I'm not sure if it's a good design, but that's how PyGMT works now.

We can catch the error and set API_IMAGE_LAYOUT to a null value, but I feel it's easier and better to do it in GMT instead.

joa-quim commented 3 years ago

Changing the GMT code to

    else if (!strncmp (keyword, "API_IMAGE_LAYOUT", 16U))   /* Report image/band layout */
#ifdef HAVE_GDAL
        gmt_M_memcpy (value, API->GMT->current.gdal_read_in.O.mem_layout, 4, char);
#else
        value[0] = '\0';
#endif

should work. But you are introducing a compatibility break because this would show up only in GMT6.2 If it was me I would certainly deal with this on the PyGMT side since it makes no sense asking for a info of something that is not used and might not exist.

seisman commented 3 years ago

If it was me I would certainly deal with this on the PyGMT side since it makes no sense asking for a info of something that is not used and might not exist.

Not a bad suggestion. I will see if it's difficult to implement in PyGMT.

seisman commented 3 years ago

Closed as it's possible to deal with it in PyGMT.