Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove expected failures on grdview tests #589

Merged
merged 11 commits into from
Sep 9, 2020
Merged

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Sep 7, 2020

Description of proposed changes

Removes the expected failures (xfail) pytest marks used as a workaround in #503 to deal with grdview failures.

Part 1 to fixing #451. Part 2 will be to deal with xfails on makecpt and other grd* modules.

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 weiji14 added the maintenance Boring but important stuff for the core devs label Sep 7, 2020
The xarray based slicing isn't as precise as grdcut, so some images were a few pixels off when plotted in 3D.
@weiji14 weiji14 marked this pull request as ready for review September 7, 2020 09:19
Comment on lines 187 to 197
@check_figures_equal()
def test_grdview_with_perspective_and_zaxis_frame(gridfile, grid):
"""
Run grdview by passing in a grid and plotting an annotated vertical
z-axis frame.
"""
fig = Figure()
fig.grdview(grid=grid, perspective=[225, 30], zscale=0.005, frame="zaf")
return fig
fig_ref = Figure()
fig_ref.grdview(grid=gridfile, perspective=[225, 30], zscale=0.005, frame="zaf")
fig_test = Figure()
fig_test.grdview(grid=grid, perspective=[225, 30], zscale=0.005, frame="zaf")
return fig_ref, fig_test
Copy link
Member Author

@weiji14 weiji14 Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test_grdview_with_perspective_and_zaxis_frame is a very strange one. It passes when the full test suite is ran using make test, but was failing locally for me everytime I ran pytest --mpl --verbose --doctest-modules pygmt/tests/test_grdview.py. Even weirder is that the problem seems to be on the NetCDF plot rather than the xarray one (now I've seen everything)! The difference is a doubling on the x and y-axis frame, will report this to upstream GMT in a bi (Edit: it's at GenericMappingTools/gmt#4181):

NetCDF (expected, but wrong) xarray.DataArray (test, correct?!!)
test_grdview_with_perspective_and_zaxis_frame-expected test_grdview_with_perspective_and_zaxis_frame

Exact error is:

matplotlib.testing.exceptions.ImageComparisonFailure: Image sizes do not match expected size: (1534, 2664, 3) actual size (1521, 2638, 3)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The xarray.DataArray result is "incorrect", because GMT takes the input xarray grid as a Cartesian grid, although grid.gmt.gtype=1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so there's several layers of 'wrong-ness' here it seems? The NetCDF grdview plot is wrong because it has a double frame (without the zebra stripes), and the xarray grid is wrong because it's using the Cartesian plain map frame? But these are 'absolute' wrongs i.e. bugs, and our new @check_figures_equal decorator only does a 'relative' comparison to ensure we're reproducing GMT plots in PyGMT (be it right or wrong).

What I don't understand is why the tests pass when ran using make test (false negative), but fails with pytest --mpl --verbose --doctest-modules pygmt/tests/test_grdview.py. Seems like another flaky test issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it puzzles me, too.

@seisman
Copy link
Member

seisman commented Sep 7, 2020

Just two suggestions:

  1. perhaps rename grid to xgrid. xgrid is more clear that it's a xarray grid.
  2. The codes for the reference and test images are the same, except the input is different. So we have a lot of duplicated codes. I'm wondering if we could rewrite the tests like:
fig = [None, None]
for i, grid in enumerate((gridfile, xgrid)):
    fig[i] = Figure()
    fig[i].grdview(grid=gridfile, perspective=[135, 15])
return fig
# return fig[0], fig[1]

@weiji14 weiji14 changed the title Remove xfails on grdview tests Remove expected failures on grdview tests Sep 7, 2020
Otherwise GMT (fig_ref) might plot double-lined x and y axis frame, while PyGMT (fig_test) plots a single-lined frame.
.gitignore Outdated Show resolved Hide resolved
@weiji14 weiji14 merged commit 89857c3 into master Sep 9, 2020
@weiji14 weiji14 deleted the remove_grdview_xfails branch September 9, 2020 01:09
weiji14 added a commit that referenced this pull request Apr 1, 2021
Related to #1131, and is almost like reverting #589.
@weiji14 weiji14 mentioned this pull request Apr 1, 2021
5 tasks
weiji14 added a commit that referenced this pull request Apr 1, 2021
Related to #1131, and is almost like reverting #589.
weiji14 added a commit that referenced this pull request May 26, 2021
Final round of tests to migrate to DVC for the
`grdview` plotting module. This is related to #1131,
and is almost like reverting #589 actually.

* Migrate test_grdview baseline images to dvc
* Update fig.grdview baseline images for GMT 6.2.0rc1
* Plot fancy frame for test_grdview_with_perspective
* Update fig.grdview baseline images for GMT 6.2.0rc2
* Use projection="Q15c+" for test_grdview_with_perspective
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Final round of tests to migrate to DVC for the
`grdview` plotting module. This is related to GenericMappingTools#1131,
and is almost like reverting GenericMappingTools#589 actually.

* Migrate test_grdview baseline images to dvc
* Update fig.grdview baseline images for GMT 6.2.0rc1
* Plot fancy frame for test_grdview_with_perspective
* Update fig.grdview baseline images for GMT 6.2.0rc2
* Use projection="Q15c+" for test_grdview_with_perspective
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants