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

Refactor pygmt.surface tests #1568

Merged
merged 20 commits into from
Nov 14, 2021
Merged

Refactor pygmt.surface tests #1568

merged 20 commits into from
Nov 14, 2021

Conversation

maxrjones
Copy link
Member

@maxrjones maxrjones commented Oct 4, 2021

Description of proposed changes

This PR captures the warnings from using short aliases in the surface test, in order to test that the warnings are generated properly and to reduce dilution of the useful information in the pytest reports.

Fixes #1460

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

@maxrjones maxrjones added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Oct 4, 2021
@seisman seisman added this to the 0.5.0 milestone Oct 5, 2021
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Oct 5, 2021
pygmt/tests/test_surface.py Outdated Show resolved Hide resolved
@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Oct 5, 2021
@maxrjones maxrjones changed the title Use pytest.warns for surface test of short aliases Refactor pygmt.surface tests Oct 5, 2021
@maxrjones maxrjones marked this pull request as draft October 5, 2021 17:36
@maxrjones maxrjones marked this pull request as ready for review October 5, 2021 21:00
@willschlitzer willschlitzer mentioned this pull request Oct 7, 2021
35 tasks
pygmt/tests/test_surface.py Outdated Show resolved Hide resolved
pygmt/tests/test_surface.py Outdated Show resolved Hide resolved
pygmt/tests/test_surface.py Outdated Show resolved Hide resolved
@willschlitzer
Copy link
Contributor

@GenericMappingTools/pygmt-maintainers Are we waiting on the changes for preprocessed data as mentioned in this comment above? Or should we approve this after a decision is made on how to check the values in the returned array? If we are waiting on both changes, I suspect it won't be done by Friday.

@maxrjones
Copy link
Member Author

@GenericMappingTools/pygmt-maintainers Are we waiting on the changes for preprocessed data as mentioned in this comment above? Or should we approve this after a decision is made on how to check the values in the returned array? If we are waiting on both changes, I suspect it won't be done by Friday.

I can make both changes for v0.6.0.

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.

I still see warnings in the log at https://github.com/GenericMappingTools/pygmt/runs/4109194553?check_suite_focus=true#step:11:624 😅:

___________________________ test_surface_input_file ____________________________
----------------------------- Captured stderr call -----------------------------
Warning: WARNING]: 1 unusable points were supplied; these will be ignored.
Warning: WARNING]: You should have pre-processed the data with block-mean, -median, or -mode.
Warning: WARNING]: Check that previous processing steps write results with enough decimals.
Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.
________________________ test_surface_input_data_array _________________________
----------------------------- Captured stderr call -----------------------------
Warning: WARNING]: 1 unusable points were supplied; these will be ignored.
Warning: WARNING]: You should have pre-processed the data with block-mean, -median, or -mode.
Warning: WARNING]: Check that previous processing steps write results with enough decimals.
Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.
____________________________ test_surface_input_xyz ____________________________
----------------------------- Captured stderr call -----------------------------
Warning: WARNING]: 1 unusable points were supplied; these will be ignored.
Warning: WARNING]: You should have pre-processed the data with block-mean, -median, or -mode.
Warning: WARNING]: Check that previous processing steps write results with enough decimals.
Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.
_______________________ test_surface_with_outgrid_param ________________________
----------------------------- Captured stderr call -----------------------------
Warning: WARNING]: 1 unusable points were supplied; these will be ignored.
Warning: WARNING]: You should have pre-processed the data with block-mean, -median, or -mode.
Warning: WARNING]: Check that previous processing steps write results with enough decimals.
Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.
__________________ test_surface_deprecate_outfile_to_outgrid ___________________
----------------------------- Captured stderr call -----------------------------
Warning: WARNING]: 1 unusable points were supplied; these will be ignored.
Warning: WARNING]: You should have pre-processed the data with block-mean, -median, or -mode.
Warning: WARNING]: Check that previous processing steps write results with enough decimals.
Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.

pygmt/helpers/testing.py Outdated Show resolved Hide resolved
@maxrjones
Copy link
Member Author

Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.

Yes this is the IEEE rounding problem because the data are not high precision, whereas the previous tests had warnings due to a lack of preprocessing. I suggest we just use verbose="e"

@weiji14
Copy link
Member

weiji14 commented Nov 4, 2021

Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.

Yes this is the IEEE rounding problem because the data are not high precision, whereas the previous tests had warnings due to a lack of preprocessing. I suggest we just use verbose="e"

I see. Or maybe we could play around with a different spacing (e.g. 1.5, 2, 2.5) and see if that works?

@maxrjones
Copy link
Member Author

Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.

Yes this is the IEEE rounding problem because the data are not high precision, whereas the previous tests had warnings due to a lack of preprocessing. I suggest we just use verbose="e"

I see. Or maybe we could play around with a different spacing (e.g. 1.5, 2, 2.5) and see if that works?

If you think only reporting errors using verbose="e" is a bad idea, I can play around with the spacing and region. But, it would involve more than just changing the code in pygmt/test/test_surface.py since the current region and spacing were already used to generate the new file on the GMT server. Not that big of a deal, but I don't want to spend too much more effort here if it's not needed.

@weiji14
Copy link
Member

weiji14 commented Nov 9, 2021

Warning: WARNING]: Possibly some data were half-way between nodes and subject to IEEE 754 rounding.

Yes this is the IEEE rounding problem because the data are not high precision, whereas the previous tests had warnings due to a lack of preprocessing. I suggest we just use verbose="e"
I see. Or maybe we could play around with a different spacing (e.g. 1.5, 2, 2.5) and see if that works?

If you think only reporting errors using verbose="e" is a bad idea, I can play around with the spacing and region. But, it would involve more than just changing the code in pygmt/test/test_surface.py since the current region and spacing were already used to generate the new file on the GMT server. Not that big of a deal, but I don't want to spend too much more effort here if it's not needed.

I think your time is more valuable elsewhere, so let's just go with verbose="e" then.

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Nov 9, 2021
@maxrjones maxrjones merged commit 1b74259 into main Nov 14, 2021
@maxrjones maxrjones deleted the test-surface-short-aliases branch November 14, 2021 19:43
@maxrjones maxrjones removed the final review call This PR requires final review and approval from a second reviewer label Nov 14, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
* Refactor surface tests to use preprocessed data and xarray testing

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Michael Grund <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
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.

Improve surface tests by checking values and using pre-processed data
5 participants