Skip to content

Raise RuntimeWarning instead of an exception for irregular grid spacing#1530

Merged
seisman merged 6 commits intomainfrom
irreular-grid-inc
Oct 4, 2021
Merged

Raise RuntimeWarning instead of an exception for irregular grid spacing#1530
seisman merged 6 commits intomainfrom
irreular-grid-inc

Conversation

@seisman
Copy link
Member

@seisman seisman commented Sep 21, 2021

Description of proposed changes

GMT only supports regular grid spacing. So when passing a grid to GMT API, it's always assumed that the grid has regular spacings in x and y dimensions.

When irregular grid spacings are detected, we raised an exception, but the detection may be too strict (see #1468 for the issue report). In this PR, a warning is raised, instead of an exception.

Fixes #1468

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

@seisman seisman added the bug Something isn't working label Sep 21, 2021
@seisman seisman added this to the 0.5.0 milestone Sep 21, 2021
@@ -95,9 +97,12 @@ def dataarray_to_matrix(grid):
coord_incs = coord[1:] - coord[0:-1]
coord_inc = coord_incs[0]
if not np.allclose(coord_incs, coord_inc):
Copy link
Member

Choose a reason for hiding this comment

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

Could we try to work out the np.assert_allclose atol or rtol argument values which won't matter for GMT? I remember reading somewhere that GMT uses single-precision (4-byte/32-bit) floating point to store the grids (see https://github.com/GenericMappingTools/gmt/blob/32296e512b173cfdd2c3e8da1d2735e49b79f02a/doc/rst/source/explain_float.rst_#L5-L7). So how many decimal places is that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Although GMT grids are stored using 4-bytes, the grid metadata (range, inc, and others) are stored using double-precision floating points (8 bytes) (see https://github.com/GenericMappingTools/gmt/blob/8ee9060d8620a7d0e599e3f4990303b1517aed02/src/gmt_resources.h#L413).

I'm looking at @leouieda's example data in #1468. The "longitude" dimension has the following values:

>>> grid.coords["longitude"].values
array([-180.    , -179.8333, -179.6667, ...,  179.6667,  179.8333,
        180.    ], dtype=float32)

As you can see, the grid spacings can be either 0.1666 or 0.1667 even if we ignore the floating-point precision issue.

Comment on lines 97 to 98
coord_incs = coord[1:] - coord[0:-1]
coord_inc = coord_incs[0]
Copy link
Member

Choose a reason for hiding this comment

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

If the coordinate increments can be slightly different, is it safe to just take the first one, or should we take the mean/average value (inside the if not np.allclose(coord_incs, coord_inc): block)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, using the mean spacing sounds better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can calculate the increment from the minimum, maximum and length of the coordinates? Take the coordinates in #1530 (comment) as an example, the array grid.coords["longitude"].values has a size of 2161, and the increment is (180-(-180))/(2161-1) = 0.1666666...

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can calculate the increment from the minimum, maximum and length of the coordinates? Take the coordinates in #1530 (comment) as an example, the array grid.coords["longitude"].values has a size of 2161, and the increment is (180-(-180))/(2161-1) = 0.1666666...

Implemented in d5d98c8.

@weiji14 weiji14 changed the title Raise a warning intsead an exception for irregular grid spacing Raise RuntimeWarning intstead of an exception for irregular grid spacing Sep 21, 2021
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.

Would prefer if @leouieda could give this a review and see if it fixes his problem at #1468.

@weiji14 weiji14 requested a review from leouieda September 30, 2021 23:23
@weiji14 weiji14 changed the title Raise RuntimeWarning intstead of an exception for irregular grid spacing Raise RuntimeWarning instead of an exception for irregular grid spacing Oct 3, 2021
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Oct 3, 2021
@seisman seisman merged commit 7bd5a8f into main Oct 4, 2021
@seisman seisman deleted the irreular-grid-inc branch October 4, 2021 07:11
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 4, 2021
@leouieda
Copy link
Member

leouieda commented Oct 4, 2021

Hi everyone, sorry just got to this now. I tried out the latest version on main and it seems to be working great! Thank you for implementing this @seisman and @weiji14 ❤️

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…ng (GenericMappingTools#1530)

* Raise a warning intsead an exception for irregular grid spacing
* Calcuate grid spacing if irregular spacing is detected
* Update pygmt/tests/test_clib.py

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PyGMT is too strict when checking for irregular grids

3 participants