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

WIP: Update 6 baseline images due to GMT 6.1.0 changes #522

Closed
wants to merge 4 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented Jul 14, 2020

This PR updates the baseline images of 6 failing tests and removes the xfail markers.

These tests fail due to slight changes between GMT 6.0 and GMT 6.1, and are not related to grid data.

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.

@seisman seisman added the maintenance Boring but important stuff for the core devs label Jul 14, 2020
@seisman seisman added this to the 0.2.x milestone Jul 14, 2020
@seisman seisman added the skip-changelog Skip adding Pull Request to changelog label Jul 14, 2020
@seisman seisman force-pushed the update-baseline-images branch from 1a550f7 to b1401cf Compare July 14, 2020 01:29
@seisman seisman requested a review from weiji14 July 14, 2020 01:48
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

A bit hesitant to approve this but ok.

We should look into using https://matplotlib.org/3.2.2/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal for the grd* changes, i.e. check that PyGMT is reproducing the images output by the current GMT version, otherwise this is a never-ending game.

@seisman
Copy link
Member Author

seisman commented Jul 14, 2020

OK. We can hold this PR for a while.

@seisman seisman changed the title Update 6 baseline images due to GMT 6.1.0 changes WIP: Update 6 baseline images due to GMT 6.1.0 changes Jul 14, 2020
@seisman
Copy link
Member Author

seisman commented Jul 22, 2020

We should look into using https://matplotlib.org/3.2.2/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal for the grd* changes, i.e. check that PyGMT is reproducing the images output by the current GMT version, otherwise this is a never-ending game.

I'm a little confused here. When I first read the link, I thought we can avoid unnecessary image updates by increasing the RMS threshold, but I don't see why check_figures_equal is preferred over image_comparison.

One more thing, we are using the pytest-mpl plugin, but the link above is for matplotlib.

@weiji14
Copy link
Member

weiji14 commented Jul 22, 2020

I'm a little confused here. When I first read the link, I thought we can avoid unnecessary image updates by increasing the RMS threshold, but I don't see why check_figures_equal is preferred over image_comparison.

One more thing, we are using the pytest-mpl plugin, but the link above is for matplotlib.

Sorry for the confusion. I wanted to point out how matplotlib states that

"This [check_figures_equal] decorator should be preferred over image_comparison when possible in order to keep the size of the test suite from ballooning.".

This is a totally different way of testing, different to using pytest-mpl. I.e. Instead of testing against a static baseline image, I'm considering whether we should just check the differences between 1. a plot generated using xarray.DataArray grids and 2. NetCDF file grids. The risk here is that there's a bug which affects both xarray.DataArray and NetCDF grids, but currently all our problems stem from using xarray.DataArray grids. If we assume that NetCDF grids are always fine in GMT, then this would be safe to do.

@seisman
Copy link
Member Author

seisman commented Jul 22, 2020

check the differences between 1. a plot generated using xarray.DataArray grids and 2. NetCDF file grids.

It sounds a really good idea.

@seisman
Copy link
Member Author

seisman commented Aug 5, 2020

We should look into using https://matplotlib.org/3.2.2/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal for the grd* changes, i.e. check that PyGMT is reproducing the images output by the current GMT version, otherwise this is a never-ending game.

Tried a little bit of the mpl decorator. It doesn't work for us, because it has some matplotlib-specific codes. But the idea is great. I think we need to implement our own way to compare two figures, like:

fig1 = pygmt.Figure()
fig1.basemap(....)

fig2 = pygmt.Figure()
fig2.basemap(....)

pygmt.check_figure_equal(fig1, fig2, tol="0.01")

@seisman seisman modified the milestones: 0.2.0, 0.2.1 Sep 17, 2020
@seisman seisman closed this Oct 15, 2020
@seisman seisman deleted the update-baseline-images branch October 15, 2020 18:03
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 skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants