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

Support passing data in numpy int8, int16, uint8 and uint16 dtypes to GMT #1963

Merged
merged 4 commits into from
Jun 16, 2022

Conversation

seisman
Copy link
Member

@seisman seisman commented Jun 15, 2022

Description of proposed changes

Support numpy data types np.int8, np.int16, np.uint8, and np.uint16.

References:

Fixes #1960.

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.

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 this to the 0.7.0 milestone Jun 15, 2022
@seisman seisman mentioned this pull request Jun 15, 2022
@seisman seisman added the enhancement Improving an existing feature label Jun 15, 2022
@seisman seisman changed the title Support passing more data types to GMT Support passing more numpy dtypes to GMT Jun 15, 2022
@weiji14
Copy link
Member

weiji14 commented Jun 15, 2022

Awesome @seisman! Didn't realize GMT even supported these other dtypes!

Just need to update some tests, e.g. these lines:

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

Also 6 more lines in test_clib.py. Maybe a good idea to make the dtypes list into a pytest fixture, use something like:

Use dtypes = "int8 int16 int32 int64 uint8 uint16 uint32 uint64 float32 float64".split()

@seisman
Copy link
Member Author

seisman commented Jun 15, 2022

Awesome @seisman! Didn't realize GMT even supported these other dtypes!

Just need to update some tests, e.g. these lines:

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

dtypes = "float32 float64 int32 int64 uint32 uint64".split()

Also 6 more lines in test_clib.py. Maybe a good idea to make the dtypes list into a pytest fixture, use something like:

Use dtypes = "int8 int16 int32 int64 uint8 uint16 uint32 uint64 float32 float64".split()

Was working on it. Now pushed to this branch.

Comment on lines 863 to 864
Not at all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.
Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized that passing str_ type matrix to GMT is allowed in the PyGMT put_matrix function, but was not documented here. But I'm wondering what happens if passing a str matrix to GMT.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Not at all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.
Not all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.

I just realized that passing str_ type matrix to GMT is allowed in the PyGMT put_matrix function, but was not documented here. But I'm wondering what happens if passing a str matrix to GMT.

Maybe try and see?

pygmt/clib/session.py Outdated Show resolved Hide resolved
pygmt/clib/session.py Outdated Show resolved Hide resolved
Comment on lines 863 to 864
Not at all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Not at all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.
Not all numpy dtypes are supported, only: int8, int16, int32, int64,
uint8, uint16, uint32, uint64, float32, float64, str\_ and datetime64.

I just realized that passing str_ type matrix to GMT is allowed in the PyGMT put_matrix function, but was not documented here. But I'm wondering what happens if passing a str matrix to GMT.

Maybe try and see?

@weiji14
Copy link
Member

weiji14 commented Jun 15, 2022

Just as an aside, is there any chance of GMT supporting half precision floats? I.e. https://numpy.org/doc/stable/reference/arrays.scalars.html#numpy.float16, or 2-byte floats. Not sure if it's possible in C, but supporting float16 will be useful for some machine learning workloads that use float16.

@seisman
Copy link
Member Author

seisman commented Jun 15, 2022

Looking at the source code of the GMT_Put_Matrix function (https://github.com/GenericMappingTools/gmt/blob/b552be3a3f76455cd45f32202d468d14ce93cd28/src/gmt_api.c#L15458) and the gmtapi_insert_vector function (https://github.com/GenericMappingTools/gmt/blob/b552be3a3f76455cd45f32202d468d14ce93cd28/src/gmt_api.c#L15297), I think only numeric data types are supported.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
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.

Looks ok to me, but maybe wait and see if @De-Guo has time to verify that it fixes his issue at #1960.

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Jun 15, 2022
@seisman seisman marked this pull request as ready for review June 15, 2022 16:10
@seisman
Copy link
Member Author

seisman commented Jun 15, 2022

Just as an aside, is there any chance of GMT supporting half precision floats? I.e. numpy.org/doc/stable/reference/arrays.scalars.html#numpy.float16, or 2-byte floats. Not sure if it's possible in C, but supporting float16 will be useful for some machine learning workloads that use float16.

I have no idea about float16. Better to open a feature request in the GMT repository.

@seisman seisman changed the title Support passing more numpy dtypes to GMT Support passing data in numpy int8, int16, uint8 and uint16 dtypes to GMT Jun 15, 2022
@seisman seisman merged commit 9f9b7f6 into main Jun 16, 2022
@seisman seisman deleted the add-datatypes branch June 16, 2022 10:51
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Jun 16, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
… GMT (GenericMappingTools#1963)

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
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uint8 datatype issue
2 participants