Let data processors default to gridline reg for unspecified remote grids

[PaulWessel]

If users select a remote data set without specifying the registration then grid-processing modules shall select gridline registration while plot producers shall (continue to) select pixel registration. This is to avoid issues such as #6678.

Currently, this policy only applied to grdtrack. Note I am on a train and cannot run the tests, and we should discuss if we want to make this change. Hence WIP.

[joa-quim]

OK, hadn't realize that this applies only to the cases of remote files and when the user didn't bother (quite likely because he/she doesn't even knows the matter) to explicitly specify the registration. In that case we are free to set a default behavior.

[maxrjones]

This disagrees with the description here - https://docs.generic-mapping-tools.org/6.3/datasets/remote-data.html#usage. If the PR is merged we should adjust the description in that section and add a deprecation notice to the changelog since it is technically breaking backwards compatibility. I'm not opposed to the change, but think it should be applied to all processing modules if merged.

[PaulWessel]

True, but it is more of a documentation bug. For instance, it does not mention that grdtrack is using gridline registration. It basically only talks about registration in the context of plots. Given that it is a fools errand for anyone to not specify registration with a grid processor (meaning they probably have no idea what they are really doing), I think it is better to declare a bug and clarify in that documentation section how we handle registration for grid processing and that we suggest users specify what they want. It also adds consistency rather than just singling out grdtrack.

[maxrjones]

True, but it is more of a documentation bug. For instance, it does not mention that grdtrack is using gridline registration. It basically only talks about registration in the context of plots. Given that it is a fools errand for anyone to not specify registration with a grid processor (meaning they probably have no idea what they are really doing), I think it is better to declare a bug and clarify in that documentation section how we handle registration for grid processing and that we suggest users specify what they want. It also adds consistency rather than just singling out grdtrack.

I think we're in agreement that we need better documentation. I agree with having the same default for all processors and add a +1 for your previous suggestion to raise a warning when the registration is not provided. Even very smart people can write data processing scripts without specifying the registration (e.g., #6678).

[PaulWessel]

I will work this up tomorrow for review.

[PaulWessel]

Testing this branch I run into hung jobs apparently - cannot tell if it is Hawaii server network or what. If you build this branch, does things like

gmt grdcut @earth_relief_06m -R179/196/49/61 -Gc.nc

give any trouble (probably you need to delete your saved file first.

[PaulWessel]

So turns out I can run that command manually, but for some reason my tests get hung up on some of the scripts and it is usually one with grdcut on a remote file without resolution specification.

[PaulWessel]

There are about 30 tests that access @earth_relief_** without registration from non-plotting modules (grdcut, grdclip, grdsample, grdfilter, grdgravmag3d, grdmath). All of these used to get pixel-grids but now they get gridline grids and hence we get plot failures.

I propose to edit those scripts and append _p so that we will still get the pixel grids so those tests will pass. The exception will be the new gridfill.sh script where we will do the gridline-registered grid as discussed. I assume you are OK with this rather than updating 30 PS files in DVC?

[maxrjones]

Yes, simpler to addend _p for most of them.

[PaulWessel]

I will do that once the surface-faster branch is merged (soon - awaiting CI).

[PaulWessel]

I am thinking that while the _p trick shall be used for various tests, I am hesitant to change 6 example and tutorial scripts to do this. Here I think perhaps updating the PS is the better solution. Thoughts?

[maxrjones]

Fine with me to update the PS for the user-facing examples.

[PaulWessel]

So there are some dumb things in the tutorial which has not kept up with the times. For instance, we just translated old scripts to modern mode but that meant we do

1. Use grdcut to extract some topo
2. Run grdgradient to build intensity grid
3. Run grdimage to make the map

For instance, here is tut 16:

gmt begin GMT_tut_16
	gmt set GMT_THEME cookbook
	gmt makecpt -Crainbow -T1000/5000
	gmt grdcut @earth_relief_30s_p -R-108/-103/35/40 -Gtut_relief.nc
	gmt grdgradient tut_relief.nc -Ne0.8 -A100 -fg -Gus_i.nc
	gmt grdimage @earth_relief_30s -R-108/-103/35/40 -Ius_i.nc -JM6i -B -BWSnE
	gmt colorbar -DJTC -I0.4 -Bxa -By+lm
gmt end show

But surely, in a tutorial to show simple things, this should instead just be:

gmt begin GMT_tut_16_new
	gmt set GMT_THEME cookbook
	gmt grdimage @earth_relief_30s -R-108/-103/35/40 -I+a100+ne0.8 -JM6i -B -BWSnE
	gmt colorbar -DJTC -I0.4 -Bxa -By+lm
gmt end show

No? Otherwise we just complicate things by having to use _p in the grdcut step to ensure the two grids are compatible (ie., the unspecified resolution in grdimage).

[PaulWessel]

Basically, I propose to rewrite the text surrounding tutorial examples 16 and 19 so we avoid this painful section of steps. We can still talk about grdgradient and what it does, but then say for most purposes like here we simply do it implicitly via -I.

[PaulWessel]

I have acted on my impulses and updated the old-style examples in the tutorial. I have left most/all the doc and example scripts without explicit resolution and hence updated those PS files in DVC, and fixed the test scripts. I havd removed the WIP label.

@seisman, let me know if the CI is unhappy. about anything,

[seisman]

@seisman, let me know if the CI is unhappy. about anything,

Building on macOS CI is failing as reported in #6718. Still unclear which homebrew package breaks.

I've enabled the full tests and docs builds in a05fd94. Will see if anything else break.

[seisman]

Comment the "pull_request" line before merging.

[seisman]

There is one failing test on Linux and Windows:

1/1 Test #465: test/grdfilter/openmp.sh .........***Failed   55.79 sec
Set GMT_SESSION_NAME = 69616
Warning: ING]: Remote dataset given to a data processing module but no registration was specified - default to gridline registration (if available)
Warning: WARNING]: Your -D option was interpreted to mean -Dx
/usr/bin/gm compare: image difference exceeds limit (0.0455962 > 0.003).
test/grdfilter/openmp.ps: RMS Error = 0.0456 [FAIL]
memtrack errors: 0
exit status: 1

Here is the diff image:

[PaulWessel]

OK, not having omp so did not notice. I added the _p in the grdfilter call so now it should be as before - let's see.

[PaulWessel]

Let me know how it went, @seisman. I fly back to Hawaii on the weekend for a 2 week break, then back to Oxford, so things may slow down in 24 hours...

[seisman]

The grdfill test still fails on Windows, same as #6678 (comment).

[PaulWessel]

OK, this latest commit deals with several somewhat related and unrelated issues:

1. The switch to set gridline-registration as default was ephemeral and got reset once the command line files were processed, so by the time -Ag option in grdfill was examined we were back at pixel registration when GMT_File_Path came around the second time. Hence earth_relief_01d_p.grd was picked.
2. Since we know the region we should pass -Rw/e/s/n into grdtrack since otherwise it will read the entire globe and waste time/memory. This is now done.
3. The test script had one reference to @earth_relief_20m_holes.grd and another without the leading @.
4. The plot changed so dvc has been updated.
5. The grdfill module was thought to be a PS producer since its output type was unclear.

Once again probably need full tests, @seisman.

[seisman]

Tests pass.