Region for 3s tiles fails if not multiple of 3s #4124

PaulWessel · 2020-08-31T00:12:40Z

See form post for details. This PR simply adds a failing test.

PaulWessel · 2020-08-31T00:15:49Z

Note: I thought I had fixed that since it was also reported earlier but I cannot find the pull request of issue. Can you, @seisman?

PaulWessel · 2020-08-31T00:16:57Z

Note it works without -I but fails with -I.

seisman · 2020-08-31T00:20:25Z

This one? #3206

PaulWessel · 2020-08-31T00:28:06Z

Thanks yes. OK, so this current case does not involve 15s so perhaps takes a different path. I will debug.

If an intensity grid is given, and a remote earth_relief grid is given, then when the intensity grid is read in and we specify the subregion wesn, make sure this region matches the possibly adjusted tile_wesn due to rounding in gmt_remote.c See #4124 for background.

* Make sure intensity subset matches tiled region if given If an intensity grid is given, and a remote earth_relief grid is given, then when the intensity grid is read in and we specify the subregion wesn, make sure this region matches the possibly adjusted tile_wesn due to rounding in gmt_remote.c See #4124 for background. * Update grdimage.c * Update grdimage.c Actually, similar problem cased the issue to begin with.

* Make sure intensity subset matches tiled region if given If an intensity grid is given, and a remote earth_relief grid is given, then when the intensity grid is read in and we specify the subregion wesn, make sure this region matches the possibly adjusted tile_wesn due to rounding in gmt_remote.c See #4124 for background. * Update grdimage.c * Update grdimage.c Actually, similar problem cased the issue to begin with. Co-authored-by: Paul Wessel <[email protected]>

PaulWessel · 2020-08-31T03:45:56Z

Fixed by #4130.

seisman · 2020-08-31T03:47:49Z

But do we want to add the test for it?

PaulWessel · 2020-08-31T03:53:46Z

Ooops, yes, this PR has the test script and now the correct PS.

seisman · 2020-08-31T03:58:08Z

One concern about this PR is, this PR uses earth_relief_03. It means we have to cache the earth_relief_03s grid (at least some tiles).

PaulWessel · 2020-08-31T04:00:46Z

Might we have a flag to prevent a test from being run in the CI, only locally?

PaulWessel · 2020-09-02T02:18:50Z

Likewise, perhaps remove backport?

PaulWessel · 2020-09-02T02:39:33Z

Before adding another single flag, perhaps thing about deeper about test partitioning. Ways to consider subsets of tests:

Ability to skip or select tests that are CPU/time intensive
Ability to skip or select tests that require large data sets
Ability to skip or select tests only for the API (the api dir)
Ability to select tests only for one module (i.e., only the test/module folder)

Might be others. Hopefully the flag (maybe this is a bitflag) can help the CI know what to pre-download? A key issue for us is often the time it takes. I made a histogram of run times for the tests once. It is long-tailed, with mostly quick tests and a few slow ones. A linear scheme does not seem sensible, maybe a flag 1, 2 , 3 that maps to log10 of some scaled time?

seisman · 2020-09-02T03:06:34Z

Ability to skip or select tests only for the API (the api dir)
Ability to select tests only for one module (i.e., only the test/module folder)

It's already supported. ctest -R api to run api tests only, and ctest -R psxy to run psxy-specific tests.

Currently, tests have inconsistent names (e.g., GMT_hammer.sh in doc/scripts directory, ex01/ex01.sh in doc/examples directory, and pshistogram/flip.sh in test directory). In #3949, I plan to let all tests use their relative path as the test name. For example, the test name should be doc/scripts/GMT_hammer.sh instead of GMT_hammer.sh. After #3949 is done, it's possible to split all tests into three groups: examples, scripts in the documentation, and the real tests.

seisman · 2020-09-02T03:37:30Z

test/grdimage/rounding.sh

+# Failing script while fixing https://forum.generic-mapping-tools.org/t/error-pygmt-gmtcliberror-module-grdimage-failed-with-status-code-78/829
+# Now reported as issue ###


Please fix/improve the comments.

seisman · 2020-09-02T03:39:30Z

For this test, I think we can run gmt grdcut -R3:57/4:18/44:00/44:15 @earth_relief_03s -G/dev/null to cache the two tiles (~3 MB) in the CI.

PaulWessel · 2020-09-02T04:55:27Z

Like this?

seisman · 2020-09-02T04:58:30Z

test/grdimage/rounding.sh

+# Ensure the two tiles are downloaded first:
+gmt grdcut -R3:57/4:18/44:00/44:15 @earth_relief_03s -G/dev/null


No. I didn't mean this. It should be done in the CI script:

gmt/ci/azure-pipelines-mac.yml

Lines 80 to 90 in 66f5bdc

# Download remote files, if not already cached

- bash: |

set -x -e

# list of datasets used in tests

data="@earth_relief_01d \

@earth_relief_30m \

@earth_relief_20m \

@earth_relief_15m \

@earth_relief_10m @earth_relief_10m_g \

@earth_relief_06m \

@earth_relief_05m @earth_relief_05m_g \

OK, I agree. Should I or you move that code to the .yml file then?

I'll do that.

Regino for 3s tiles fails if not multiple of 2s

199c0e0

See form post for details. This PR simply adds a failing test.

PaulWessel added bug Something isn't working backport 6.1 Backport this PR to 6.1 branch labels Aug 31, 2020

PaulWessel requested review from joa-quim and seisman August 31, 2020 00:12

PaulWessel self-assigned this Aug 31, 2020

PaulWessel mentioned this pull request Aug 31, 2020

Bad rounding when earth_relief_03s is used due to filler 15s grid #4129

Closed

PaulWessel mentioned this pull request Aug 31, 2020

Make sure intensity subset matches tiled region if given #4130

Merged

PaulWessel added 2 commits August 30, 2020 17:30

Merge branch 'master' into tile-rounding

b0ea009

Add orig PS

97e5f0f

PaulWessel closed this Aug 31, 2020

PaulWessel reopened this Aug 31, 2020

seisman removed the backport 6.1 Backport this PR to 6.1 branch label Sep 2, 2020

seisman reviewed Sep 2, 2020

View reviewed changes

Pre-download two 3s tiles

c166a8b

seisman reviewed Sep 2, 2020

View reviewed changes

seisman added 2 commits September 2, 2020 01:48

Cache two tiles of earth_relief_03s data

a394c1a

Remove one unnecessary comment

afd0e0f

seisman approved these changes Sep 2, 2020

View reviewed changes

PaulWessel merged commit e5acedc into master Sep 2, 2020

PaulWessel deleted the tile-rounding branch September 2, 2020 06:15

		# Failing script while fixing https://forum.generic-mapping-tools.org/t/error-pygmt-gmtcliberror-module-grdimage-failed-with-status-code-78/829
		# Now reported as issue ###

		# Ensure the two tiles are downloaded first:
		gmt grdcut -R3:57/4:18/44:00/44:15 @earth_relief_03s -G/dev/null

	# Download remote files, if not already cached
	- bash: \|
	set -x -e
	# list of datasets used in tests
	data="@earth_relief_01d \
	@earth_relief_30m \
	@earth_relief_20m \
	@earth_relief_15m \
	@earth_relief_10m @earth_relief_10m_g \
	@earth_relief_06m \
	@earth_relief_05m @earth_relief_05m_g \

Region for 3s tiles fails if not multiple of 3s #4124

Region for 3s tiles fails if not multiple of 3s #4124

Uh oh!

Conversation

PaulWessel commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

seisman commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

seisman commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

seisman commented Aug 31, 2020

Uh oh!

PaulWessel commented Aug 31, 2020

Uh oh!

PaulWessel commented Sep 2, 2020

Uh oh!

PaulWessel commented Sep 2, 2020

Uh oh!

seisman commented Sep 2, 2020

Uh oh!

seisman Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

seisman commented Sep 2, 2020

Uh oh!

PaulWessel commented Sep 2, 2020

Uh oh!

seisman Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

PaulWessel Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

seisman Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants