Migrate Figure.grdimage tests to use dvc #1169

willschlitzer · 2021-04-03T08:20:59Z

Per #1131, this pull request moves reference images to use dvc and replaces tests that use @check_figures_equal. It also removes some tests, including those that use pytest.mark.parameterize to repeatedly pass different arguments to a test.

Description of proposed changes

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.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

… set one projection type and central meridian

weiji14 · 2021-04-03T08:40:03Z

pygmt/tests/test_grdimage.py

-@pytest.mark.parametrize("lon0", [0, 123, 180])
-@pytest.mark.parametrize("proj_type", ["H", "W"])
-def test_grdimage_central_meridians(grid, proj_type, lon0):
+def test_grdimage_central_meridians(grid):


Err, could you revert this change please. This parametrize test was set up explicitly at #560 (comment) to catch a bug (#390).

Will do; I had just assumed it was repeatedly testing the same arguments. On that note, how should this handle a reference image then, since it produces 6 different images?

Also, why don't we test this on all plotting modules that have a projection parameter? I assume it's the same issue?

Actually, I would recommend to keep this test on @check_figures_equal for now because it's quite a serious bug we want resolved at some point (differences between plotting NetCDF and xarray grids should not exist).

Sounds good. Should I leave the other tests that use the parameterize decorator with check_figures_equal as well?

Yes, just for now please 😄

Should be fixed (took me too long to realize there would be errors when I merged with GitHub)!

github-actions · 2021-04-04T07:40:00Z

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status	Path
added	pygmt/tests/baseline/test_grdimage.png
added	pygmt/tests/baseline/test_grdimage_file.png
added	pygmt/tests/baseline/test_grdimage_over_dateline.png
added	pygmt/tests/baseline/test_grdimage_slice.png

Image diff(s)

pygmt/tests/baseline/test_grdimage.png

pygmt/tests/baseline/test_grdimage_file.png

pygmt/tests/baseline/test_grdimage_over_dateline.png

pygmt/tests/baseline/test_grdimage_slice.png

Report last updated at commit 543c3b8

…image-dvc

weiji14 · 2021-04-04T21:17:03Z

Let's hold on to this for a while @willschlitzer, there seems to be a bug at #1172 for both fig.grdimage and fig.grdview with plotting geographic grids. I'll re-mark it as draft for now.

willschlitzer · 2021-04-08T20:21:11Z

Let's hold on to this for a while @willschlitzer, there seems to be a bug at #1172 for both fig.grdimage and fig.grdview with plotting geographic grids. I'll re-mark it as draft for now.

@weiji14 It looks like the upstream bug has been fixed, according to the comment from @seisman. Should I mark this as ready for review, or are we still waiting on additional changes?

weiji14 · 2021-04-08T20:56:16Z

Let's hold on to this for a while @willschlitzer, there seems to be a bug at #1172 for both fig.grdimage and fig.grdview with plotting geographic grids. I'll re-mark it as draft for now.

@weiji14 It looks like the upstream bug has been fixed, according to the comment from @seisman. Should I mark this as ready for review, or are we still waiting on additional changes?

The question is, do we want to 1) store these 'incorrect' GMT 6.1.1 images now, or 2) wait for GMT 6.2.0rc1 to come out and use that to create the 'correct' grdimage test images.

willschlitzer · 2021-04-09T06:23:30Z

Let's hold on to this for a while @willschlitzer, there seems to be a bug at #1172 for both fig.grdimage and fig.grdview with plotting geographic grids. I'll re-mark it as draft for now.

@weiji14 It looks like the upstream bug has been fixed, according to the comment from @seisman. Should I mark this as ready for review, or are we still waiting on additional changes?

The question is, do we want to 1) store these 'incorrect' GMT 6.1.1 images now, or 2) wait for GMT 6.2.0rc1 to come out and use that to create the 'correct' grdimage test images.

I'll defer to you on this one. In my opinion (which I don't feel strongly about), this test should be merged as the reference images are "wrong" because of a GMT problem, not a PyGMT one. Since, as I understand it, we'll have to update most of our reference images when we switch to GMT 6.2, I figure we can update the grdimage and grdview images then, along with the other tests that are affected by GMT themes.

Side note: How will testing using reference images work if we're still on PyGMT v0.3.1, as we'll want to be be testing our compatibility with both GMT 6.1.1 and GMT 6.2 that may produce different images from each other, but only have a single reference image per test?

weiji14

The question is, do we want to 1) store these 'incorrect' GMT 6.1.1 images now, or 2) wait for GMT 6.2.0rc1 to come out and use that to create the 'correct' grdimage test images.

I'll defer to you on this one. In my opinion (which I don't feel strongly about), this test should be merged as the reference images are "wrong" because of a GMT problem, not a PyGMT one. Since, as I understand it, we'll have to update most of our reference images when we switch to GMT 6.2, I figure we can update the grdimage and grdview images then, along with the other tests that are affected by GMT themes.

Ok, let's merge this in then, and we can update the baseline images when GMT 6.2.0 is out (the GMT 6.2.0rc1 release should be close 🤞). The grdview test migration to dvc doesn't quite work but that's not relevant to this PR.

Side note: How will testing using reference images work if we're still on PyGMT v0.3.1, as we'll want to be be testing our compatibility with both GMT 6.1.1 and GMT 6.2 that may produce different images from each other, but only have a single reference image per test?

Well, I'd like to think that PyGMT v0.3.1 should work with GMT 6.2.0 for 99% of use cases, and if it doesn't, people should just upgrade to GMT 6.2.0 👿. By 'work', I mean run without crashing, not that the exact same figure is produced (a user won't typically care if their image is a few pixels different between versions). Yes, in a perfect world, we would have a matrix build with GMT 6.1.1 and GMT 6.2.0, and baseline images being pulled from the DVC remote matching these 2 versions, but that's a lot of work to maintain.

Anyways, LGTM!

seisman · 2021-04-10T04:19:57Z

The upstream bug #1172 should not affect these images, because these tests all explicitly specify geographic projections.

willschlitzer · 2021-04-10T09:53:20Z

Anyways, LGTM!

Felt particularly old when I had to look up what "LGTM" means.

weiji14 · 2021-04-10T10:34:52Z

Anyways, LGTM!

Felt particularly old when I had to look up what "LGTM" means.

I like to think of it as "Let's get this merged" 🔀 Wait until you try looking up GMT.

willschlitzer · 2021-04-10T13:57:02Z

Anyways, LGTM!

Felt particularly old when I had to look up what "LGTM" means.

I like to think of it as "Let's get this merged" 🔀 Wait until you try looking up GMT.

The internet told me it was "looks good to me" which works too!

Definitely have typed in GMT to Google and realized it most certainly wasn't the GMT I was looking for 😂

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

remove pytest.mark.parameterize from test_grdimage_central_meridians;…

ea0ce73

… set one projection type and central meridian

willschlitzer added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Apr 3, 2021

willschlitzer added this to the 0.4.0 milestone Apr 3, 2021

weiji14 reviewed Apr 3, 2021

View reviewed changes

Merge branch 'master' into test-grdimage-dvc

dc015ff

vercel bot temporarily deployed to Preview April 4, 2021 07:31 Inactive

move reference images to dvc, add dvc files to git

8b6b317

willschlitzer added 2 commits April 4, 2021 08:43

move reference images to dvc, add dvc files to git

49ffd0e

Merge remote-tracking branch 'origin/test-grdimage-dvc' into test-grd…

4aa75f5

…image-dvc

vercel bot temporarily deployed to Preview April 4, 2021 07:44 Inactive

willschlitzer added 2 commits April 4, 2021 08:47

re-add paramterize for test_grdimage_central_meridian

c0efbfd

re-add paramters for test_grdimage_central_meridian

291590f

vercel bot temporarily deployed to Preview April 4, 2021 07:52 Inactive

willschlitzer changed the title ~~WIP: Migrate fig.grdimage tests to use dvc~~ Migrate fig.grdimage tests to use dvc Apr 4, 2021

willschlitzer marked this pull request as ready for review April 4, 2021 08:06

weiji14 marked this pull request as draft April 4, 2021 21:17

weiji14 approved these changes Apr 10, 2021

View reviewed changes

Merge branch 'master' into test-grdimage-dvc

543c3b8

vercel bot temporarily deployed to Preview April 10, 2021 03:24 Inactive

weiji14 marked this pull request as ready for review April 10, 2021 03:24

seisman approved these changes Apr 10, 2021

View reviewed changes

seisman changed the title ~~Migrate fig.grdimage tests to use dvc~~ Migrate Figure.grdimage tests to use dvc Apr 10, 2021

seisman merged commit 82ebbbc into master Apr 10, 2021

seisman deleted the test-grdimage-dvc branch April 10, 2021 17:45

weiji14 mentioned this pull request May 26, 2021

Migrate tests to use dvc-tracked baseline images #1131

Closed

28 tasks

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022

Migrate Figure.grdimage tests to use dvc (GenericMappingTools#1169)

0435c95

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Figure.grdimage tests to use dvc #1169

Migrate Figure.grdimage tests to use dvc #1169

willschlitzer commented Apr 3, 2021

weiji14 Apr 3, 2021

willschlitzer Apr 3, 2021 •

edited

Loading

weiji14 Apr 3, 2021

willschlitzer Apr 3, 2021 •

edited

Loading

weiji14 Apr 3, 2021

willschlitzer Apr 4, 2021

github-actions bot commented Apr 4, 2021 •

edited

Loading

weiji14 commented Apr 4, 2021

willschlitzer commented Apr 8, 2021

weiji14 commented Apr 8, 2021

willschlitzer commented Apr 9, 2021 •

edited

Loading

weiji14 left a comment

seisman commented Apr 10, 2021

willschlitzer commented Apr 10, 2021

weiji14 commented Apr 10, 2021

willschlitzer commented Apr 10, 2021 •

edited

Loading

Migrate Figure.grdimage tests to use dvc #1169

Migrate Figure.grdimage tests to use dvc #1169

Conversation

willschlitzer commented Apr 3, 2021

weiji14 Apr 3, 2021

Choose a reason for hiding this comment

willschlitzer Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

weiji14 Apr 3, 2021

Choose a reason for hiding this comment

willschlitzer Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

weiji14 Apr 3, 2021

Choose a reason for hiding this comment

willschlitzer Apr 4, 2021

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2021 • edited Loading

Summary of changed images

Image diff(s)

weiji14 commented Apr 4, 2021

willschlitzer commented Apr 8, 2021

weiji14 commented Apr 8, 2021

willschlitzer commented Apr 9, 2021 • edited Loading

weiji14 left a comment

Choose a reason for hiding this comment

seisman commented Apr 10, 2021

willschlitzer commented Apr 10, 2021

weiji14 commented Apr 10, 2021

willschlitzer commented Apr 10, 2021 • edited Loading

willschlitzer Apr 3, 2021 •

edited

Loading

willschlitzer Apr 3, 2021 •

edited

Loading

github-actions bot commented Apr 4, 2021 •

edited

Loading

willschlitzer commented Apr 9, 2021 •

edited

Loading

willschlitzer commented Apr 10, 2021 •

edited

Loading