-
Notifications
You must be signed in to change notification settings - Fork 233
Wrap GMT_Put_Strings to pass str columns into GMT C API directly #520
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
Changes from 7 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
c811e65
Move test_put_* from test_clib to test_clib_put
weiji14 5c5f152
Initial wrapper for GMT_Put_Strings C API function
weiji14 d7c3053
Merge branch 'master' into gmt_put_strings
weiji14 fccedae
Try using GMT_TEXT for numpy string types
weiji14 07b95ed
Remove column argument from put_strings function
weiji14 88eef3a
Set valid GMT data mode as GMT_IS_OUTPUT
weiji14 6390e04
Try passing "GMT_IS_VECTOR" family type to put_strings
weiji14 42af00e
Do `put_vector` x and y before `put_strings` s, dim is 2 only
weiji14 9ef04b0
Refactor text to use virtualfile_from_vectors instead of pandas tempfile
weiji14 ab1e041
Split test_clib_put into separate _strings, _matrix and _vector files
weiji14 01754fc
Try using ctypes.POINTER in argtypes of c_put_strings
weiji14 452d3d3
Merge branch 'master' into gmt_put_strings
seisman 9c8dad9
Merge branch 'master' into gmt_put_strings
seisman 6dff9ba
Merge branch 'master' into gmt_put_strings
seisman cfb9124
Merge branch 'master' into gmt_put_strings
seisman 2485437
Pass strings using "GMT_IS_VECTOR|GMT_IS_DUPLICATE"
seisman 7fa1f0c
Merge branch 'master' into gmt_put_strings
seisman e8e7768
Use GMT_IS_VECTOR|GMT_IS_DUPLICATE when calling put_strings
seisman 2f0c798
Improve the put_strings test
seisman 6d6ea24
Merge branch 'master' into gmt_put_strings
seisman 9d063b3
Add back import pandas as pd
weiji14 2d81f76
Merge branch 'master' into gmt_put_strings
seisman 95afa90
Merge branch 'master' into gmt_put_strings
weiji14 ed0dce6
Revert refactor text to use virtualfile_from_vectors
weiji14 aeabd8e
Add test for passing in one string column to virtualfile_from_vectors
weiji14 abcbcf7
Refactor virtualfile_from_vectors to handle up to 2 string columns
weiji14 129135e
Fix test_plot_datetime by not passing first two columns into put_strings
weiji14 4f1ccda
Refactor to handle any number of string type columns
weiji14 30c18eb
Test for put_strings failing to increase code coverage
weiji14 62469cb
Expect failures for tests using GMT_Put_strings on GMT < 6.1.1
weiji14 26f51c9
Fix an incorrect note in virtualfile_from_vectors
weiji14 726de44
Concatenate last string columns instead of allowing arbitrary positions
weiji14 4c99477
Test variable length strings
weiji14 2f9d689
Replace gmt info with select in test_virtualfile_from_vectors_str_cols
weiji14 fe794c9
Fix truncated strings and an incorrect test
weiji14 f59d970
Replace gmt select with convert in test_virttualfile_from_vectors_str…
weiji14 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I tried this following your datetime example, but I think (?) it's functionally equivalent to
strings.ctypes.data_as(ctp.c_char_p)? Either way it runs, but the string data doesn't seem to get written to the virtualfile so the test fails. I've also triedstrings.ctypes.data_as(ctp.POINTER(ctp.c_char_p))but it doesn't seem to make a difference. 😕There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings.ctypes.data_as(ctp.c_char_p)may not work.What I did is adding a print statement inside the GMT_Put_Strings function, printing the string arrays passed to it. Using
strings.ctypes.data_as(ctp.c_char_p)crashes immediately for me, but using my code can print the strings correctly, but then crash 🤦♂️.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using:
Does it crash for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it still crashes, but I can see that the strings are correctly passed to the GMT_Put_Strings function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
strings.ctypes.data_as(ctp.POINTER(ctp.c_char_p))? Are the strings correctly passed in?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I was just taking a look at it (thanks for tracking down the bug by the way!). Sounds like we'll need to pin to GMT > 6.1.1 for the next release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not 100% sure GenericMappingTools/gmt#3718 is related to the issue here. I just had the feeling that the string array may be freed by Python when GMT tries to read it.
Hopefully, GMT 6.1.1 can fix issue and #515.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, GenericMappingTools/gmt#3718 is already merged into GMT master, and will be backported to 6.1 branch soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I'll test it out when I get the time (need to prepare some stuff for an online conference next week). Still need to wait on the grid problem at #515 but that's a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just triggered the GMT master branch testing. Ideally, we should only see one failure from
test_put_strings.