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

Re-caching GMT remote files #495

Closed
wants to merge 2 commits into from
Closed

Re-caching GMT remote files #495

wants to merge 2 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented Jun 26, 2020

Description of proposed changes

The earth relief data for GMT 6.0.0 was just changed back to gridline registration. It means that GMT<=6.0.0 uses gridline-registered earth relief data, while GMT>=6.1.0 will uses pixel-registered earth relief data by default.

Related to #451.

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
Copy link
Member

weiji14 commented Jun 26, 2020

Great, it's just 23 failures instead of 43 now. All of the makecpt ones are passing again. Let's not update the baseline images or numbers just yet, I want to make sure the tests will be forward compatible with #476 and GMT6.1.0 (specifically that we treat the earth_relief grids as Geographic instead of Cartesian).

@weiji14 weiji14 added the maintenance Boring but important stuff for the core devs label Jun 26, 2020
@seisman
Copy link
Member Author

seisman commented Jun 26, 2020

Most of the 23 failures are easy to fix, except two test_grdimage and test_grdimage_slice. The two failures don't make sense.

The following Python script gives me a white-black map with GMT 6.0.0:

import pygmt
from pygmt.datasets import load_earth_relief
pygmt.print_clib_info()

grid = load_earth_relief()
fig = pygmt.Figure()
fig.grdimage(grid, cmap='earth', projection='W0/6i', V='l')
fig.show(method="external")

Output:
image

Warnings:

grdimage [WARNING]: gmt_grd_project: Output grid extrema [-1.57645e+06/1.0734e+06] exceed extrema of input grid [-8592.5/5559] due to resampling
grdimage [WARNING]: gmt_grd_project: See option -n+c to clip resampled output range to given input range

It even doesn't work with PyGMT v0.1.1 + GMT 6.0.0.

@weiji14
Copy link
Member

weiji14 commented Jun 26, 2020

Yeah, I was getting that a while ago too, sometimes it's purple 😆. I think it's fine in GMT master though.

@seisman
Copy link
Member Author

seisman commented Jun 27, 2020

Yes, but when we release PyGMT v0.1.1, everything is fine with GMT 6.0.0.

I checkout PyGMT v0.1.1, and test it with the GMT 6.0.0 I built locally, I still get the white-black map.

@seisman
Copy link
Member Author

seisman commented Jun 27, 2020

Just realized that the two tests test_grdimage and test_grdimage_slice pass in the CI jobs, but fail locally for me.

@seisman
Copy link
Member Author

seisman commented Jun 27, 2020

FYI, the two tests test_grdimage and test_grdimage_slice suddenly pass locally for me. Perhaps I messed up with my local environment yesterday.

@weiji14
Copy link
Member

weiji14 commented Jun 27, 2020

Hmm, might be that you've cached the pixel registered grids locally, and the cache was just invalidated today. Probably best to run gmt clear cache to be sure.

@seisman
Copy link
Member Author

seisman commented Jun 27, 2020

Hmm, might be that you've cached the pixel registered grids locally, and the cache was just invalidated today. Probably best to run gmt clear cache to be sure.

I can't reproduce the failures anymore. They all work well for me.

Perhaps I can update the baseline images and also the numbers to make all the CI green but not merge the branch. It seems easier for us to check GMT master and #476, and decide if we really want a v0.1.2 release.

@weiji14
Copy link
Member

weiji14 commented Jun 28, 2020

Perhaps I can update the baseline images and also the numbers to make all the CI green but not merge the branch. It seems easier for us to check GMT master and #476, and decide if we really want a v0.1.2 release.

Do you mean just cherry-picking the changes here on top of whatever branch we're working on?

@seisman
Copy link
Member Author

seisman commented Jun 28, 2020

Here are some tests I just did locally.

Currently, there are 23 failures for GMT 6.0.0. To make all the tests pass, I need to update the following files:

pygmt/clib/session.py
pygmt/tests/baseline/test_grdcontour.png
pygmt/tests/baseline/test_grdcontour_interval_file_full_opts.png
pygmt/tests/baseline/test_grdcontour_labels.png
pygmt/tests/baseline/test_grdcontour_slice.png
pygmt/tests/baseline/test_grdview_on_a_plane.png
pygmt/tests/baseline/test_grdview_on_a_plane_styled_with_facadepen.png
pygmt/tests/baseline/test_grdview_on_a_plane_with_colored_frontal_facade.png
pygmt/tests/baseline/test_grdview_surface_plot_styled_with_contourpen.png
pygmt/tests/baseline/test_grdview_with_cmap_for_perspective_surface_plot.png
pygmt/tests/baseline/test_grdview_with_cmap_for_surface_monochrome_plot.png
pygmt/tests/baseline/test_grdview_with_perspective_and_zaxis_frame.png
pygmt/tests/baseline/test_grdview_with_perspective_and_zscale.png
pygmt/tests/baseline/test_grdview_with_perspective_and_zsize.png
pygmt/tests/test_datasets.py
pygmt/tests/test_grdinfo.py
pygmt/tests/test_grdtrack.py

After these changes, all tests pass. I then temporarily changed @earth_relief_01d to @earth_relief_01d_g (and all other similar dataset) so that I won't see any failures due to grid registration differences. However, I still see 20 failures and have to update the following 15 images:

  1. test_basemap_polar.png
  2. test_coast_iceland.png
  3. test_colorbar_positioned_using_justification_code.png
  4. test_config.png
  5. test_config_font_annot.png
  6. test_config_font_one.png
  7. test_grdview_drapegrid_dataarray.png
  8. test_grdview_grid_file_with_region_subset.png
  9. test_grdview_surface_mesh_plot_styled_with_meshpen.png
  10. test_grdview_surface_plot_styled_with_contourpen.png
  11. test_grdview_with_cmap_for_image_plot.png
  12. test_grdview_with_cmap_for_perspective_surface_plot.png
  13. test_grdview_with_cmap_for_surface_monochrome_plot.png
  14. test_grdview_with_perspective_and_zaxis_frame.png
  15. test_makecpt_to_plot_grid_scaled_with_series.png

Among the 15 failures:

Then I updated the 15 images above, cherry-picked the changes in PR #476 (assuming this branch is correct), and ran the tests again. I saw the following failing images:

  1. test_grdview_drapegrid_dataarray.png
  2. test_grdview_grid_dataarray.png
  3. test_grdview_on_a_plane.png
  4. test_grdview_on_a_plane_styled_with_facadepen.png
  5. test_grdview_on_a_plane_with_colored_frontal_facade.png
  6. test_grdview_surface_mesh_plot_styled_with_meshpen.png
  7. test_grdview_surface_plot_styled_with_contourpen.png
  8. test_grdview_with_cmap_for_image_plot.png
  9. test_grdview_with_cmap_for_perspective_surface_plot.png
  10. test_grdview_with_cmap_for_surface_monochrome_plot.png
  11. test_grdview_with_perspective.png
  12. test_grdview_with_perspective_and_zaxis_frame.png
  13. test_grdview_with_perspective_and_zscale.png
  14. test_grdview_with_perspective_and_zsize.png
  15. test_gridimage_over_dateline.png

The 14 grdview tests are failing due to the same reason (unknown yet). Here is an example.
Baseline image
image

New image if PR #476 is merged:
image

Perhaps because the default projection depends on the grid type (either GMT_GRID_IS_GEO or GMT_GRID_IS_CARTESIAN.

@weiji14
Copy link
Member

weiji14 commented Jun 28, 2020

Among the 15 failures:

  • 1, 2, 3 are caused by tiny changes in GMT 6.1.0;

  • 4, 5, 6 are surprising to me, possibly related to GenericMappingTools/gmt#3523. Need to check if it's a GMT bug before GMT 6.1.0 is released.

Ok, I think we can agree to not merge in any baseline png image changes, in case anything happens before GMT 6.1.0 release.

  • 7-15 are big differences caused by how CPT hinges are handled in GMT 6.1.0 (GenericMappingTools/gmt#2416). [These grdview images are already updated to make GMT 6.0.0 pass].

Yes, good spotting, we'll definitely need to update these makecpt png images then, unless we want to go for a more future proof solution (i.e. not have the tests depend on earth_relief data). I'm actually quite keen for this bugfix as I've had to manually copy those changed cpt files to make the figures in my last manuscript...

Then I updated the 15 images above, cherry-picked the changes in PR #476 (assuming this branch is correct), and ran the tests again. I saw the following failing images:

The 14 grdview tests are failing due to the same reason (unknown yet). Here is an example.

Perhaps because the default projection depends on the grid type (either GMT_GRID_IS_GEO or GMT_GRID_IS_CARTESIAN.

I wouldn't assume that the branch is correct yet 😆, I still need to make sure things work if node_offset is 0 or missing from the xarray.DataArray. But looking at the two png images, it does like like the projection has changed somewhat. The new image looks to be a bit more curved.

Just to add that it might be possible to keep the same old grdview baseline images if we force those tests to use GMT_GRID_IS_CARTESIAN, at least for the non-coloured ones not using the oleron cpt.

@seisman
Copy link
Member Author

seisman commented Jun 28, 2020

Perhaps the best way to minimize image updates is,

  1. Fix the tiny failures about the numbers but don't update any images
  2. Release PyGMT v0.1.2 immediately after GMT 6.1.0 is released, even though some tests still fail due to incorrect baseline images. The PyGMT v0.1.2 should work for both GMT 6.0.0 and 6.1.0 (tests can fail but no crashes).
  3. Bump the minimum required GMT version 6.1.0 and update images.

@weiji14
Copy link
Member

weiji14 commented Jun 28, 2020

Perhaps the best way to minimize image updates is,

  1. Fix the tiny failures about the numbers but don't update any images

I'm okay with this. Could do it in this PR even. I was trying to figure out how to parametrize the tests to use earth_relief_01d on GMT 6.0 and earth_relief_01d_g for GMT 6.1 but it got real messy, and there's too much noise with the failing tests now.

  1. Release PyGMT v0.1.2 immediately after GMT 6.1.0 is released, even though some tests still fail due to incorrect baseline images. The PyGMT v0.1.2 should work for both GMT 6.0.0 and 6.1.0 (tests can fail but no crashes).

No strong opinion on this, I don't really mind when PyGMT v0.1.2 gets released, either before or after GMT 6.1.0 is fine. We should make sure to pin to GMT <= 6.1.0 on conda-forge, i.e. PyGMT v0.1.2 is the last version to support GMT 6.0, and contains some minor changes to make it compatible with GMT 6.1.

3. Bump the minimum required GMT version 6.1.0 and update images.

Looking forward to this 🚀

@weiji14 weiji14 added the skip-changelog Skip adding Pull Request to changelog label Jun 28, 2020
@weiji14
Copy link
Member

weiji14 commented Jul 16, 2020

I think we can close this since Azure Pipelines isn't used anymore and we've updated to GMT 6.1.0 in #507.

@weiji14 weiji14 closed this Jul 16, 2020
@seisman seisman deleted the update-cache branch July 16, 2020 03:17
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.

None yet

2 participants