Skip to content

Allow GMT_Put_Vector to convert other text items than datetime #4849

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

Merged
merged 12 commits into from
Feb 28, 2021

Conversation

PaulWessel
Copy link
Member

@PaulWessel PaulWessel commented Feb 24, 2021

See this post for background. While we added support for GMT_DATETIME last year, this PR generalizes the conversion from text to numbers via the type GMT_TEXT. The strings may be either Cartesian numbers, datetime, longitude, or latitude, and we also allow these to be converted to other internal representations than GMT_DOUBLE (e.g., GMT_FLOAT, GMT_LONG, etc.) via logical addition (e.g., pass type as GMT_TEXT|GMT_FLOAT).

I have left the prior scheme of GMT_DATETIME for datetime so we are backwards compatible, but one now only needs to select GMT_TEXT and GMT figures out what you passed, hopefully. We assume each column has the same type and there is no error checking if you pass both time and longitude as part of a single column. I added testapi_putvector.c and calling script apiputvector.sh which demonstrates that it works.

I think a quick test and this can be approved and merged.

While we already support a type of GMT_DATETIME, this PR adds support for incoming text strings via GMT_TEXTLON, GMT_TEXTLAT, and GMT_TEXT (Cartesian values).
@PaulWessel PaulWessel self-assigned this Feb 24, 2021
@seisman
Copy link
Member

seisman commented Feb 24, 2021

Just built this branch and ran the full PyGMT tests, the GMT_DATETIME implementation works like before.

@PaulWessel
Copy link
Member Author

Great, maybe @weiji14 could test Lon lat inputs

@seisman
Copy link
Member

seisman commented Feb 24, 2021

As for the new data types, GMT_TEXTLON, GMT_TEXTLAT, and GMT_TEXT, does it mean that PyGMT has to know the data types before passing to GMT?

For example, if the string is 12.34E, PyGMT must check if it's text, longitude, latitude or datetime, and then choose the correct type when calling GMT_Put_Vector. It means more work for PyGMT (and other external wrappers) to detect the data types.

I'm wondering if we can simply pass any strings (e.g., 12.34, 12.34E, 12.34N, 12:30:30, 2020-01-01T01:00:00 to GMT giving a general data type (type=GMT_TEXT) and GMT is very clever to parse them and automatically determine the data type.

@PaulWessel
Copy link
Member Author

We could automatically determine type and give an error if more than one, e.g., both time and latitude since a column must only have one type

@PaulWessel PaulWessel changed the title WIP Allow GMT_Put_Vector to convert other text items than datetime Allow GMT_Put_Vector to convert other text items than datetime Feb 24, 2021
@PaulWessel
Copy link
Member Author

@seisman, this PR is now more align with your suggestion.

@PaulWessel
Copy link
Member Author

Pinging @weiji14 and @seisman to consider this PR given it has a test that passes.

@seisman
Copy link
Member

seisman commented Feb 26, 2021

It works well with a simple test (see GenericMappingTools/pygmt#975 for the example script and output). Need to do more testing later.

So please keep the PR open for a few more days.

PaulWessel and others added 3 commits February 27, 2021 22:00
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@PaulWessel PaulWessel merged commit 5d192e4 into master Feb 28, 2021
@PaulWessel PaulWessel deleted the put-vector-extension branch February 28, 2021 19:44
@seisman
Copy link
Member

seisman commented Feb 28, 2021

@weiji14 Could you please bump the GMT dev version again in the conda-forge channel, so that we can better test the new GMT_TEXT data type.

@weiji14
Copy link
Member

weiji14 commented Feb 28, 2021

Ok, I'll do that in a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants