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.
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
**Breaking**: data_kind: Now 'matrix' represents a 2-D numpy array and unrecognized data types fall back to 'vectors' #3351
**Breaking**: data_kind: Now 'matrix' represents a 2-D numpy array and unrecognized data types fall back to 'vectors' #3351
Changes from 13 commits
0c82b3c
808755d
0eb4f8f
9891b2c
a9d094c
3d8be4d
7c104a9
5790923
6954c5d
9300ca3
2701a4a
7fcf57f
991f688
51569c8
4a4f192
2a6e788
2b054b6
cfa32ed
ef6e6aa
423e5dc
81e57f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 1823 in pygmt/clib/session.py
Codecov / codecov/patch
pygmt/clib/session.py#L1823
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.
Missing test coverage for these lines?
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.
I can add a test for it. Before adding more tests, I'm wondering if we should split the big "test_clib_virtualfiles.py" file (with more than 500 lines) into separate smaller test files, i.e., one test file for each Session method.
test_clib_virtualfile_in
test_clib_virtualfile_from_vectors
test_clib_virtualfile_from_matrix
test_clib_open_virtualfile
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.
We've split it before in #2784, so yes, ok to split it again 😆
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.
Done in #3512.
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.
I've added a test in cfa32ed to cover this line.
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.
Two things:
kind="matrix"
to 3-D numpy.ndarray objects?np.ndarray()
class only, because while it's not widely advertised, checking fordata is not None
would have caught other array types implementing the__array_function__
protocol (see NEP18), but checking forisinstance(data, np.ndarray)
would exclude those. That said, I'm wondering if the fallback tokind="vectors"
might just work, but need to test this out more extensively.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.
It's unclear if we can pass a 3-D numpy array yet. Even if we can, it means more work, since in
virtualfile_from_vectors
/virtualfile_from_matrix
, we explicitly check if the ndim=1 or 2. So, better to revisit the 3-D numpy array support in a separate PR if necessary.Yes, I was thinking about checking
__array_interface__
in #3351 (comment).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.
Done in 4a4f192, although other array-like objects are not tested.
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.
pandas.DataFrame now is "vectors" kind.