Complete documentation of grdimage

[weiji14]

Description of proposed changes

Time to put an end to all the noise about grdimage not being properly documented! To be fair, the documentation at https://www.pygmt.org/v0.2.0/api/generated/pygmt.Figure.grdimage.html is pretty embarrasing, considering that grdimage is one of the more widely used functions! For reference, upstream GMT docs is at https://docs.generic-mapping-tools.org/6.1/grdimage.html

Mentioned before at #364 (comment) and https://forum.generic-mapping-tools.org/t/grdgradient-in-pygmt/723/12.

Live documentation preview is at https://pygmt-git-grdimage-documentallargs.gmt.vercel.app/api/generated/pygmt.Figure.grdimage.html#pygmt.Figure.grdimage.

TODO:

Arguments/Aliases to document (see also https://www.generic-mapping-tools.org/GMT.jl/latest/#GMT.grdimage):

• A - img_out
• B - frame
• C - cmap
• D - img_in
• E - dpi
• G - color
• I - shading
• M - monochrome
• N - noclip
• Q - nan_alpha
• R - region
• U - timestamp
• V - verbose
• X - xshift
• Y - yshift (done at Add common aliases xshift (X) and yshift (Y) #624)
• f
• n - interpolation
• p - perspective (done at Add common alias perspective (p) for plotting 3D illustrations #627)
• t - transparency
• x - cores (done at Add common alias cores (x) for grdimage and other multi-threaded modules #625)

Fixes #

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst.
• Write detailed docstrings for all functions/methods.
• If adding new functionality, add an example to docstrings or tutorials.

[weiji14]

Ok, ready for review, and preview at https://pygmt-git-grdimage-documentallargs.gmt.vercel.app/api/generated/pygmt.Figure.grdimage.html#pygmt.Figure.grdimage! Best to take a closer look, I'm pretty sure I've messed up on some formatting. Also no rush to do it today since it's getting late.

This also helps with #618, documenting the fact that shading="intensfile" works. Will deal with xarray.DataArray inputs (a Python specific feature) separately another time.

[seisman]

It reads awkwardly.

Suggested change

	`grdmath` or `grdhisteq`. A third alternative is available when GMT is
	build with GDAL support. Pass **image** which can be an image file
	(geo-referenced or not). In this case the image can optionally be
	`grdmath` or `grdhisteq`. A third alternative is available when GMT is
	build with GDAL support. Pass **image** which can be an image file
	(geo-referenced or not). In this case the image can optionally be

[weiji14]

Agree that this could be worded better, currently it's just copied from upstream GMT at https://github.com/GenericMappingTools/gmt/blob/8c0b548e52938f91bfb15f47f27d023488003ef8/doc/rst/source/grdimage_common.rst_#L14-L16. Maybe we should take out the grdgradient, grdmath and grdhisteq parts since they're not wrapped in PyGMT yet?

[seisman]

I'm afraid we may forget to add them back when the wrappers are available.

[weiji14]

Maybe keep them for now, better to over-document than not. Doubt we'll see grdmath anytime soon (who uses Reverse Polish notation?!!), but grdgradient would definitely come, and grdhisteq will be a nice one for stretching colormaps.

[seisman]

Yes, I agree.

[weiji14]

Agree that this could be worded better, currently it's just copied from upstream GMT at https://github.com/GenericMappingTools/gmt/blob/8c0b548e52938f91bfb15f47f27d023488003ef8/doc/rst/source/grdimage_common.rst_#L14-L16. Maybe we should take out the grdgradient, grdmath and grdhisteq parts since they're not wrapped in PyGMT yet?

[weiji14]

What do you think about taking out this sentence about image, related to the change at https://github.com/GenericMappingTools/pygmt/pull/620/files#r492612750.

[seisman]

What do you think about taking out this sentence about image, related to the change at https://github.com/GenericMappingTools/pygmt/pull/620/files#r492612750.

The changes are big. It's unclear to me what you mean by "taking out this sentence about image"?

[weiji14]

Oops, wrong link. Should be #620 (comment). The part where we took out grid | image.

[seisman]

The reason to take out grid | image is, it's a little unclear what grid | image means, and for the grid argument, we already mention that grid could be a image:

       The file name or a DataArray containing the input 2-D gridded data
       set or image to be plotted

[weiji14]

Yes, agree that taking grid | image is ok. But would this lead to confusion about this sentence:

        Pass **image** which can be an image file
        (geo-referenced or not). In this case the image can optionally be
        illuminated with the file provided via the *shading* option. Here, if
        image has no coordinates then those of the intensity file will be used.

I suppose we could just keep it to keep it in line with upstream GMT?

[seisman]

That's why I said these sentences don't read well in #620 (comment).

Perhaps we can change:

A third alternative is available when GMT is build with GDAL support. Pass image which can be an image file (geo-referenced or not). In this case the image can optionally be illuminated with the file provided via the -I option. Here, if image has no coordinates then those of the intensity file will be used.

to

If GMT is built with GDAL support, grid can be an image file (geo-referenced or not). In this case the image can optionally be illuminated with the file provided via the -I option. Here, if image has no coordinates then those of the intensity file will be used.

[weiji14]

Looks good enough (mentioning that image can be used with GDAL support). I'll commit the change.

[seisman]

There should be no spaces between modifier and value:

Suggested change

	grid via a call to `grdgradient`; append **+a** \\ *azimuth*,
	**+n** \\ *args*, and **+m** \\ *ambient* to specify azimuth,
	grid via a call to `grdgradient`; append **+a**\\ *azimuth*,
	**+n**\\ *args*, and **+m**\\ *ambient* to specify azimuth,

Are you sure we need double back-slashes?

[weiji14]

Good catch. Single backslash would result in a flake8 error W605 invalid escape sequence '\ '

[seisman]

Single backslash would result in a flake8 error W605 invalid escape sequence '\ '

Double backslashes in ReST syntax doesn't give what we want if they're not in docstring: http://rst.ninjs.org/#KiorYSoqXFwgKmF6aW11dGgq

Why does flake8 check ReST syntax? Is it a flake8 bug? Or can we make flake8 ignore it for ReST strings?

[seisman]

Anyway, the documentation looks good now. We may see if we can avoid double backslashes in the future.

[weiji14]

Double backslashes in ReST syntax doesn't give what we want if they're not in docstring: http://rst.ninjs.org/#KiorYSoqXFwgKmF6aW11dGgq

The double backslash is needed for Python, the alternative would be to use a raw string (e.g. r" **+n**\ *args* "). We came across this at #525 (comment), might be good if we can standardize this in our CONTRIBUTING.md document (open an issue?).

Anyways, the rendered version looks fine:

Why does flake8 check ReST syntax? Is it a flake8 bug? Or can we make flake8 ignore it for ReST strings?

A quick search yields https://github.com/kataev/flake8-rst, but I haven't tried it, and don't know if it does what we want.

[seisman]

GMT uses singe backslash syntax like **+a**\ *azimuth*. If we copy documentation from GMT, then I expect we will see the same warning often.

[weiji14]

I'll open up an issue for this to continue the discussion :)

[weiji14]

Hmm, there's a strange failure on the Ubuntu Python 3.6 test at https://github.com/GenericMappingTools/pygmt/pull/620/checks?check_run_id=1152171757 with a missing earth_relief grid. I'll re-run just to be sure before merging.