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

Fix tests for Aug 2023 updated remote datasets #2636

Merged
merged 14 commits into from
Aug 31, 2023
Merged

Fix tests for Aug 2023 updated remote datasets #2636

merged 14 commits into from
Aug 31, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Aug 21, 2023

Description of proposed changes

Some tests related to the GMT remote datasets are failing due to the recent updates of these datasets (see https://www.generic-mapping-tools.org/remote-datasets/changes.html for a list of these changes). The min/max values of these datasets change slightly in the new versions.

This PR fixes these failing tests by updating the min/max values.

Notes:

  1. In the tests, we use statements like npt.assert_allclose(data.min(), -180.40002, rtol=1e-5) to compare the grid value with the expected value. All datasets have a specific precision (e.g., the earth_geoid dataset has a precision of 0.01), thus it makes no sense to compare earth_geoid value with a number like -180.40002 and a relative tolerance of 1.0e-5. Instead, it makes more sense to compare the value with -180.4 and an absolute tolerance of 0.01 (the precision of the earth_geoid dataset). I've changed rtol to atol in all these tests.

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

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

@seisman seisman added the maintenance Boring but important stuff for the core devs label Aug 21, 2023
@seisman seisman added this to the 0.10.0 milestone Aug 21, 2023
@seisman
Copy link
Member Author

seisman commented Aug 21, 2023

Two synbath tests are not updated due to upstream dataset issue (GenericMappingTools/gmtserver-admin#213).

@weiji14 weiji14 changed the title Fix tests for updated datasets Fix tests for Aug 2023 updated remote datasets Aug 22, 2023
@weiji14
Copy link
Member

weiji14 commented Aug 22, 2023

Two synbath tests are not updated due to upstream dataset issue (GenericMappingTools/gmtserver-admin#213).

Ok, looks they're re-processing the synbath dataset again, let's wait a day or two to see if it gets fixed.

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.

  1. In the tests, we use statements like npt.assert_allclose(data.min(), -180.40002, rtol=1e-5) to compare the grid value with the expected value. All datasets have a specific precision (e.g., the earth_geoid dataset has a precision of 0.01), thus it makes no sense to compare earth_geoid value with a number like -180.40002 and a relative tolerance of 1.0e-5. Instead, it makes more sense to compare the value with -180.4 and an absolute tolerance of 0.01 (the precision of the earth_geoid dataset). I've changed rtol to atol in all these tests.

Cool, setting the absolute tolerance does makes more sense. Looked through each dataset and the precision values look ok, except for the ones indicated below where I couldn't find the precision documented at https://www.generic-mapping-tools.org/remote-datasets/index.html.

I'm wondering if we should file an upstream issue to https://github.com/GenericMappingTools/remote-datasets/issues to have the precision information in the metadata too? That way we could do something like npt.assert_allclose(data.min(), ..., atol=data.attrs["precision"]) instead of hardcoding a value. Just in case the precision changes in the future.

Comment on lines +40 to +41
npt.assert_allclose(data.min(), -8600.5, atol=0.5)
npt.assert_allclose(data.max(), 5559.0, atol=0.5)
Copy link
Member

Choose a reason for hiding this comment

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

Did you get the uncertainty for the earth_relief grids from the paper(s)? I'm not seeing them on https://www.generic-mapping-tools.org/remote-datasets/earth-relief.html#technical-information.

pygmt/tests/test_datasets_earth_relief.py Show resolved Hide resolved
pygmt/tests/test_datasets_earth_relief.py Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Aug 22, 2023

Looked through each dataset and the precision values look ok, except for the ones indicated below where I couldn't find the precision documented at https://www.generic-mapping-tools.org/remote-datasets/index.html.

I get the precisions from the recipes(https://github.com/GenericMappingTools/gmtserver-admin/tree/master/recipes) that were used in building the grids (the DST_SCALE parameter in the recipes).

I'm wondering if we should file an upstream issue to https://github.com/GenericMappingTools/remote-datasets/issues to have the precision information in the metadata too? That way we could do something like npt.assert_allclose(data.min(), ..., atol=data.attrs["precision"]) instead of hardcoding a value. Just in case the precision changes in the future.

I'm not sure, because these values are just the precisions that GMT used to build the grids, not exactly the real precisions of the original datasets.

@weiji14
Copy link
Member

weiji14 commented Aug 27, 2023

Looked through each dataset and the precision values look ok, except for the ones indicated below where I couldn't find the precision documented at https://www.generic-mapping-tools.org/remote-datasets/index.html.

I get the precisions from the recipes(https://github.com/GenericMappingTools/gmtserver-admin/tree/master/recipes) that were used in building the grids (the DST_SCALE parameter in the recipes).

I'm wondering if we should file an upstream issue to https://github.com/GenericMappingTools/remote-datasets/issues to have the precision information in the metadata too? That way we could do something like npt.assert_allclose(data.min(), ..., atol=data.attrs["precision"]) instead of hardcoding a value. Just in case the precision changes in the future.

I'm not sure, because these values are just the precisions that GMT used to build the grids, not exactly the real precisions of the original datasets.

Ah I see, if it's just GMT's re-gridded precisions and not the actual precisions, then it probably shouldn't be in the metadata. Let's just wait for the grids to be fixed then, seems like the earth_synbath_* grids have been fixed on the candidate server already, and just need to wait for them to be copied to the oceania server according to GenericMappingTools/gmtserver-admin#213 (comment).

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.

May need to manually regenerate the cache by uncommenting

before re-running the tests.

pygmt/tests/test_datasets_earth_relief.py Show resolved Hide resolved
pygmt/tests/test_datasets_earth_relief.py Outdated Show resolved Hide resolved
pygmt/tests/test_datasets_earth_relief.py Show resolved Hide resolved
pygmt/tests/test_datasets_earth_relief.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Aug 30, 2023

If you're short on time, I can help push the changes? Just give me a 👍

@weiji14 weiji14 marked this pull request as ready for review August 31, 2023 02:05
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.

Have updated the cache at 364c1b5 and re-ran the tests with updated minmax numbers, should all work now.

@seisman seisman merged commit b00181d into main Aug 31, 2023
14 of 17 checks passed
@seisman seisman deleted the fix-datasets branch August 31, 2023 05:28
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