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

fix dtype complex for rasterio backend #5501

Closed
wants to merge 3 commits into from

Conversation

agrouaze
Copy link

@agrouaze agrouaze commented Jun 20, 2021

rasterio backend support for the complex_int16 dtype

@pep8speaks
Copy link

pep8speaks commented Jun 20, 2021

Hello @Grouny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 4171:42: E231 missing whitespace after ','
Line 4705:37: E201 whitespace after '('
Line 4705:42: E202 whitespace before ')'
Line 4710:39: E225 missing whitespace around operator
Line 4712:35: E225 missing whitespace around operator
Line 4714:1: E302 expected 2 blank lines, found 1

Comment last updated at 2021-08-25 14:30:57 UTC

@jhamman
Copy link
Member

jhamman commented Jun 23, 2021

@Grouny - thanks for this PR! And I see that this is your first time contributing to Xarray -- Welcome!

This fix you propose looks great. Apart from the linter complaining about some whitespace, I think this would benefit from a simple test. Would you mind adding a test to xarray/tests/test_backends.py?

Q for @snowman2 - is rioxarray handling complex datatypes in this way?

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

@jhamman, there's a PR here: corteva/rioxarray#353

xarray/backends/rasterio_.py Outdated Show resolved Hide resolved
@agrouaze
Copy link
Author

agrouaze commented Jun 24, 2021

Yes I can think about a test. Could this test be a simple .tiff file opening or is it too high level?

@snowman2
Copy link
Contributor

Also see: corteva/rioxarray#359

@keewis keewis mentioned this pull request Jul 8, 2021
8 tasks
@max-sixty
Copy link
Collaborator

As discussed in the dev meeting, this would be great to merge. The test could be to create a small file, and then read it. Would that work @Grouny ?

@agrouaze
Copy link
Author

agrouaze commented Aug 25, 2021

I added a test in test_backends.py :
pytest -s ./test_backends.py::TestRasterio::test_rasterio_complex_dtype
But since there is a coming fix in rasterio I am wondering whether this PR is still needed:
rasterio/rasterio@7114fb7

@github-actions
Copy link
Contributor

Unit Test Results

         6 files           6 suites   51m 22s ⏱️
16 226 tests 14 490 ✔️ 1 736 💤 0 ❌
90 552 runs  82 368 ✔️ 8 184 💤 0 ❌

Results for commit 674d9b6.

@keewis
Copy link
Collaborator

keewis commented Aug 25, 2021

can you try if your test (without the other changes) fails with rasterio=1.2.3 and passes with rasterio>=1.2.4 (I think that's the first version that includes the commit you referenced)?

@headtr1ck headtr1ck added dependencies Pull requests that update a dependency file needs work labels Oct 12, 2022
@dcherian
Copy link
Contributor

We're about to remove the built-in rasterio backend in favour of rioxarray. Sorry for not merging this earlier @agrouaze

@dcherian dcherian closed this Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rasterio backend not supporting complex_int16 type
8 participants