Raise GMTInvalidInput exception when required z is missing #1478

yohaimagen · 2021-09-01T05:49:55Z

adding an additional if statement to the data_kind utils function that is based on a new parameter requierd_z and checks if z vector is provided alongside x and y.

Fixes #1447

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.

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

yohaimagen · 2021-09-01T06:02:06Z

Where should I add tests for not providing the z vector?

maxrjones · 2021-09-01T20:52:28Z

Where should I add tests for not providing the z vector?

I think the test should go in pygmt/tests/test_clib.py.

yohaimagen · 2021-09-02T06:25:53Z

I added the test.

If we decide to go the route of requierd_z(see discussion at #1447) we should add requierd_z = True in all the functions that do require z column. Should we do it over here or in a separate PR for each function?

maxrjones

I recommend adding required_z to the appropriate functions in this PR.

It would be good to raise the same exception if the data kind is matrix (L78) and data.shape[1] < 3 when required_z is set. Would you be able to add this to the PR and add another pytest.raises(GMTInvalidInput) check to the new test?

pygmt/helpers/utils.py

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

yohaimagen · 2021-09-03T10:37:48Z

/format

yohaimagen · 2021-09-03T11:11:00Z

I am making a list of functions that should pass required_z=True please add functions that I've missed. There are quite a lot of new functions that have been wrapped recently and will be released in 0.5 please let me know which one of them needs to be set required_z=True.

yohaimagen · 2021-09-03T11:36:51Z

Should we remove the tests of the relevant functions that test the passing of z column?

pygmt/pygmt/tests/test_contour.py

Lines 42 to 56 in cd822ca

    
           for variable in product([None, data[:, 0]], repeat=3): 
        
               # Filter one valid configuration: 
        
               if not any(item is None for item in variable): 
        
                   continue 
        
               with pytest.raises(GMTInvalidInput): 
        
                   fig.contour( 
        
                       x=variable[0], 
        
                       y=variable[1], 
        
                       z=variable[2], 
        
                       region=region, 
        
                       projection="X4i", 
        
                       color="red", 
        
                       frame="afg", 
        
                       pen="", 
        
                   )

if so we shuld extend the test to be similar to the above.

maxrjones

Nice work, thank you! 🎉

As you have seen, we leave approved pull requests up for at least a day so other maintainers have a final opportunity to check it out. In the meantime, please apply the two minor grammatical suggestions.

pygmt/tests/test_clib.py

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

yohaimagen · 2021-09-14T05:06:14Z

Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape nx1 is passed? and if so should I add it here or open another PR?

pygmt/tests/test_clib.py

seisman · 2021-09-14T06:35:32Z

Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape nx1 is passed? and if so should I add it here or open another PR?

It's more complicated, because for some modules (e.g., histogram), only one column is required.

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

seisman · 2021-09-14T23:04:10Z

Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape nx1 is passed? and if so should I add it here or open another PR?

It's more complicated, because for some modules (e.g., histogram), only one column is required.

I think this PR is good for merge and we can deal with the 2-column data check in another issue/PR.

maxrjones

I agree with @seisman that more changes aren't needed. Great work, thanks!

Following GenericMappingTools#1478.

weiji14 · 2021-09-18T22:27:39Z

pygmt/helpers/utils.py

@@ -78,6 +80,8 @@ def data_kind(data, x=None, y=None, z=None):
    elif hasattr(data, "__geo_interface__"):
        kind = "geojson"
    elif data is not None:
+        if required_z and data.shape[1] < 3:


Checking for data.shape[1] fails for xarray.Dataset inputs with AttributeError: 'Dataset' object has no attribute 'shape'. The shape attribute is valid only for numpy.ndarray, pandas.DataFrame, and maybe a few other PyData objects. Will need to create a bugfix and add some extra tests for xarray.Dataset inputs.

I can submit a PR fix since I think that I suggested this. But I feel that we should not label it as a bug since this code has not yet been included in a release, so it might be confusing to list on the changelog as a bugfix relative to v0.4.1.

Edit: Just noticed the comment on the other PR that @weiji14 would fix this. That's also fine.

Ok, I've submitted a bugfix PR at #1523. Have put on the skip-changelog label so it won't show up in the changelog :)

…ppingTools#1478) Co-authored-by: Meghan Jones <meghanj@alum.mit.edu> Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

yohaimagen added 2 commits September 1, 2021 08:37

add required_z parameters, and if statement to handle missing z vector

dc11bb6

add required_z parameters and pass it to data_kind

df1b0d5

yohaimagen added the enhancement Improving an existing feature label Sep 1, 2021

yohaimagen self-assigned this Sep 1, 2021

test requierd_z

1b7a8a6

vercel bot temporarily deployed to Preview September 2, 2021 06:22 Inactive

maxrjones reviewed Sep 3, 2021

View reviewed changes

pygmt/helpers/utils.py Outdated Show resolved Hide resolved

pygmt/helpers/utils.py Outdated Show resolved Hide resolved

pygmt/helpers/utils.py Outdated Show resolved Hide resolved

Update pygmt/helpers/utils.py

e471f9e

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

vercel bot temporarily deployed to Preview September 3, 2021 10:20 Inactive

yohaimagen and others added 2 commits September 3, 2021 13:21

Update pygmt/helpers/utils.py

11d98ee

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

Update pygmt/helpers/utils.py

bcfc03a

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

vercel bot temporarily deployed to Preview September 3, 2021 10:31 Inactive

checks that when required z data matrix has at least 3 columns

ba73d5b

[format-command] fixes

ed1419d

vercel bot temporarily deployed to Preview September 3, 2021 10:39 Inactive

yohaimagen added 4 commits September 3, 2021 14:16

add required_z=True to _blockm

166b439

add required_z=True to contour

7675449

add required_z=True to plot3d

09c68cd

add required_z=True to surface

8b3eeb2

vercel bot temporarily deployed to Preview September 3, 2021 11:18 Inactive

reformat

2a77656

vercel bot temporarily deployed to Preview September 3, 2021 11:28 Inactive

remove unnecessary import

e1d8248

vercel bot temporarily deployed to Preview September 3, 2021 11:39 Inactive

vercel bot temporarily deployed to Preview September 12, 2021 05:40 Inactive

vercel bot temporarily deployed to Preview September 12, 2021 05:48 Inactive

maxrjones reviewed Sep 13, 2021

View reviewed changes

pygmt/tests/test_clib.py Outdated Show resolved Hide resolved

pygmt/tests/test_clib.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ba10dbb

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

vercel bot temporarily deployed to Preview September 14, 2021 05:01 Inactive

Merge branch 'main' into raise_GMTInvalidInput_exception_when_z_missing

1639a5b

vercel bot temporarily deployed to Preview September 14, 2021 05:34 Inactive

seisman reviewed Sep 14, 2021

View reviewed changes

pygmt/tests/test_clib.py Outdated Show resolved Hide resolved

pygmt/tests/test_clib.py Outdated Show resolved Hide resolved

seisman added this to the 0.5.0 milestone Sep 14, 2021

Apply suggestions from code review

15075e8

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

vercel bot temporarily deployed to Preview September 14, 2021 08:49 Inactive

seisman changed the title ~~Raise gmt invalid input exception when z missing~~ Raise GMTInvalidInput exception when z missing Sep 14, 2021

seisman changed the title ~~Raise GMTInvalidInput exception when z missing~~ Raise GMTInvalidInput exception when required z is missing Sep 14, 2021

seisman approved these changes Sep 14, 2021

View reviewed changes

seisman added the final review call This PR requires final review and approval from a second reviewer label Sep 14, 2021

maxrjones approved these changes Sep 14, 2021

View reviewed changes

Merge branch 'main' into raise_GMTInvalidInput_exception_when_z_missing

91c446f

seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 15, 2021

vercel bot temporarily deployed to Preview September 15, 2021 23:30 Inactive

seisman merged commit 03d6797 into main Sep 15, 2021

seisman deleted the raise_GMTInvalidInput_exception_when_z_missing branch September 15, 2021 23:58

weiji14 added a commit to JamieJQuinn/pygmt that referenced this pull request Sep 16, 2021

Remove test_nearneighbor_input_xy_no_z

d09d63d

Following GenericMappingTools#1478.

seisman mentioned this pull request Sep 18, 2021

Wrap nearneighbor #1379

Merged

5 tasks

weiji14 reviewed Sep 18, 2021

View reviewed changes

weiji14 mentioned this pull request Sep 19, 2021

Let required_z check work on xarray.Dataset inputs #1523

Merged

5 tasks

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022

Raise GMTInvalidInput exception when required z is missing (GenericMa…

4bf6746

…ppingTools#1478) Co-authored-by: Meghan Jones <meghanj@alum.mit.edu> Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

weiji14 mentioned this pull request Jul 5, 2023

Add private function _validate_data_input to validate input data #2595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise GMTInvalidInput exception when required z is missing #1478

Raise GMTInvalidInput exception when required z is missing #1478

yohaimagen commented Sep 1, 2021 •

edited

Loading

yohaimagen commented Sep 1, 2021

maxrjones commented Sep 1, 2021

yohaimagen commented Sep 2, 2021 •

edited

Loading

maxrjones left a comment

yohaimagen commented Sep 3, 2021

yohaimagen commented Sep 3, 2021 •

edited

Loading

yohaimagen commented Sep 3, 2021

maxrjones left a comment

yohaimagen commented Sep 14, 2021

seisman commented Sep 14, 2021 •

edited

Loading

seisman commented Sep 14, 2021

maxrjones left a comment

weiji14 Sep 18, 2021 •

edited

Loading

maxrjones Sep 18, 2021 •

edited

Loading

weiji14 Sep 19, 2021

Raise GMTInvalidInput exception when required z is missing #1478

Raise GMTInvalidInput exception when required z is missing #1478

Conversation

yohaimagen commented Sep 1, 2021 • edited Loading

yohaimagen commented Sep 1, 2021

maxrjones commented Sep 1, 2021

yohaimagen commented Sep 2, 2021 • edited Loading

maxrjones left a comment

Choose a reason for hiding this comment

yohaimagen commented Sep 3, 2021

yohaimagen commented Sep 3, 2021 • edited Loading

yohaimagen commented Sep 3, 2021

maxrjones left a comment

Choose a reason for hiding this comment

yohaimagen commented Sep 14, 2021

seisman commented Sep 14, 2021 • edited Loading

seisman commented Sep 14, 2021

maxrjones left a comment

Choose a reason for hiding this comment

weiji14 Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

maxrjones Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

weiji14 Sep 19, 2021

Choose a reason for hiding this comment

yohaimagen commented Sep 1, 2021 •

edited

Loading

yohaimagen commented Sep 2, 2021 •

edited

Loading

yohaimagen commented Sep 3, 2021 •

edited

Loading

seisman commented Sep 14, 2021 •

edited

Loading

weiji14 Sep 18, 2021 •

edited

Loading

maxrjones Sep 18, 2021 •

edited

Loading