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.

Sign up for GitHub

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

Jump to bottom

Kwargs to rasterio open #5609

Merged
merged 15 commits into from
Jul 30, 2021
Merged

Kwargs to rasterio open #5609

merged 15 commits into from
Jul 30, 2021

Conversation

pkopparla
Copy link
Contributor

@pkopparla pkopparla commented Jul 15, 2021 •
edited
Loading

I've added a new kwargs argument to the function xarray.open_rasterio under xarray/xarray/backends/rasterio_.py which is passed on to the rasterio.open function as dicussed under issue #3269. I've added a line describing this change in whats-new.rst

I'm new to open source contributing, tried to follow instructions as much as I could. I am not sure how to write tests for this, would appreciate any help on that.

cheers,
Pushkar

@pep8speaks
Copy link

pep8speaks commented Jul 15, 2021 •
edited
Loading

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-27 09:23:27 UTC

@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2021 •
edited
Loading

Unit Test Results

         6 files  ±0           6 suites  ±0   48m 44s ⏱️ ±0s
16 200 tests ±0  14 475 ✔️ ±0  1 725 💤 ±0  0 ❌ ±0 
90 396 runs  ±0  82 231 ✔️ ±0  8 165 💤 ±0  0 ❌ ±0 

Results for commit 78cec7c. ± Comparison against base commit 78cec7c.

♻️ This comment has been updated with latest results.

dcherian
dcherian reviewed Jul 15, 2021
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @pkopparla This looks very nice.

I too am not sure what a test would look like> Perhaps @scottyhq or @snowman2 have ideas,

doc/whats-new.rst Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Should this pass **kwargs through rather than take an explicit kwargs arg?

@pkopparla @dcherian
Update doc/whats-new.rst
39a45e7 
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@pkopparla
Copy link
Contributor Author

pkopparla commented Jul 15, 2021 •
edited
Loading

Should this pass **kwargs through rather than take an explicit kwargs arg?

I am not sure about the nuances here, but the linked issue shows a h5netcdf function that follows the same pattern.

@keewis
Copy link
Collaborator

keewis commented Jul 15, 2021

I would try to stay as close to open_dataset as possible, which would make migrating to rioxarray's engine easier once we deprecate open_rasterio. If I understand the signature of open_dataset correctly, this is called backend_kwargs?

@dcherian
Copy link
Contributor

rioxarray.open_rasterio (and xr.open_zarr) uses the **kwargs format, so maybe that's the model to follow?

@keewis
Copy link
Collaborator

keewis commented Jul 15, 2021

right, it seems it's both backend_kwargs and **kwargs for open_dataset.

keewis
keewis reviewed Jul 15, 2021
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
@snowman2
Copy link
Contributor

For reference:

@pkopparla
Copy link
Contributor Author

Thanks everyone for the feedback. I've added a backend_kwargs and a **kwargs argument following the xarray.open_dataset style.

@pkopparla
Copy link
Contributor Author

Looks like the backend_kwargs argument is breaking the function. Should I just get rid of it and leave in **kwargs?

keewis
keewis reviewed Jul 16, 2021
View reviewed changes
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.

Should I just get rid of it and leave in **kwargs?

probably. @aurghs, @alexamici, it would be good to hear your thoughts on this if you have the time.

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

@dcherian @keewis Hey guys, a backend test is failing but I'm not sure why or how my change is affecting it. Would be grateful for a pointer.

@keewis
Copy link
Collaborator

keewis commented Jul 27, 2021

this has been fixed / worked around on main, so the merge should have fixed it

@pkopparla
Copy link
Contributor Author

pkopparla commented Jul 27, 2021 •
edited
Loading

@keewis thanks! Looks like most checks are successful now. Is there anything further I should do? Should I close the pull request?

@keewis keewis added the plan to merge Final call for comments label Jul 27, 2021
@keewis
Copy link
Collaborator

keewis commented Jul 27, 2021

no, this is fine, and the PR looks good to me. I'd say let's wait one more day to give more time for reviews and then merge by tomorrow.

@aurghs
Copy link
Collaborator

aurghs commented Jul 27, 2021 •
edited
Loading

I would try to stay as close to open_dataset as possible, which would make migrating to rioxarray's engine easier once we deprecate open_rasterio. If I understand the signature of open_dataset correctly, this is called backend_kwargs?

The idea was to deprecate in the future open_dataset backend_kwargs. Currently, in open_dataset signature, you can use either backend_kwargs or kwargs to pass the backend additional parameters. I would avoid adding backend_kwargs in open_rasterio interface.

keewis
keewis reviewed Jul 27, 2021
View reviewed changes
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.

makes sense. @pkopparla, could you remove backend_kwargs?

doc/whats-new.rst Outdated Show resolved Hide resolved
pkopparla and others added 2 commits July 27, 2021 11:18
@pkopparla @keewis
Update doc/whats-new.rst
bbda7ba 
Co-authored-by: keewis <keewis@users.noreply.github.com>
@pkopparla
Removed backend_kwargs
f0f0961
@pkopparla
Copy link
Contributor Author

@keewis Done!

@keewis keewis requested review from shoyer and jhamman July 27, 2021 10:02
@keewis
Copy link
Collaborator

keewis commented Jul 27, 2021

thanks for the update, @pkopparla. I'll merge this tomorrow if there are no further comments.

@keewis
Copy link
Collaborator

keewis commented Jul 30, 2021

Thanks, @pkopparla. I also noticed this is your first PR here. Welcome to xarray!

@keewis keewis merged commit 78cec7c into pydata:main Jul 30, 2021
@pkopparla
Copy link
Contributor Author

Thanks for leading me through the process @keewis , it's my first code contribution to any open-source project. Hope to do more in the future.

st-bender added a commit to st-bender/xarray that referenced this pull request Aug 10, 2021
@st-bender
Merge branch 'main' into repr_length_updates
ac37672 
* main: (31 commits)
  Refactor index vs. coordinate variable(s) (pydata#5636)
  pre-commit: autoupdate hook versions (pydata#5685)
  Flexible Indexes: Avoid len(index) in map_blocks (pydata#5670)
  Speed up _mapping_repr (pydata#5661)
  update the link to `scipy`'s intersphinx file (pydata#5665)
  Bump styfle/cancel-workflow-action from 0.9.0 to 0.9.1 (pydata#5663)
  pre-commit: autoupdate hook versions (pydata#5660)
  fix the binder environment (pydata#5650)
  Update api.rst (pydata#5639)
  Kwargs to rasterio open (pydata#5609)
  Bump codecov/codecov-action from 1 to 2.0.2 (pydata#5633)
  new blank whats-new for v0.19.1
  v0.19.0 release notes (pydata#5632)
  remove deprecations scheduled for 0.19 (pydata#5630)
  Make typing-extensions optional (pydata#5624)
  Plots get labels from pint arrays (pydata#5561)
  Add to_numpy() and as_numpy() methods (pydata#5568)
  pin fsspec (pydata#5627)
  pre-commit: autoupdate hook versions (pydata#5617)
  Add dataarray scatter with 3d support (pydata#4909)
  ...
dcherian added a commit to kmsquire/xarray that referenced this pull request Aug 11, 2021
@dcherian
Merge remote-tracking branch 'upstream/main' into feature/rolling-pad
61428ed 
* upstream/main: (31 commits)
  Refactor index vs. coordinate variable(s) (pydata#5636)
  pre-commit: autoupdate hook versions (pydata#5685)
  Flexible Indexes: Avoid len(index) in map_blocks (pydata#5670)
  Speed up _mapping_repr (pydata#5661)
  update the link to `scipy`'s intersphinx file (pydata#5665)
  Bump styfle/cancel-workflow-action from 0.9.0 to 0.9.1 (pydata#5663)
  pre-commit: autoupdate hook versions (pydata#5660)
  fix the binder environment (pydata#5650)
  Update api.rst (pydata#5639)
  Kwargs to rasterio open (pydata#5609)
  Bump codecov/codecov-action from 1 to 2.0.2 (pydata#5633)
  new blank whats-new for v0.19.1
  v0.19.0 release notes (pydata#5632)
  remove deprecations scheduled for 0.19 (pydata#5630)
  Make typing-extensions optional (pydata#5624)
  Plots get labels from pint arrays (pydata#5561)
  Add to_numpy() and as_numpy() methods (pydata#5568)
  pin fsspec (pydata#5627)
  pre-commit: autoupdate hook versions (pydata#5617)
  Add dataarray scatter with 3d support (pydata#4909)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/main' into sparse-unstack
7c1c099 
* upstream/main: (34 commits)
  Use same bool validator as other inputs (pydata#5703)
  conditionally disable bottleneck (pydata#5560)
  Refactor index vs. coordinate variable(s) (pydata#5636)
  pre-commit: autoupdate hook versions (pydata#5685)
  Flexible Indexes: Avoid len(index) in map_blocks (pydata#5670)
  Speed up _mapping_repr (pydata#5661)
  update the link to `scipy`'s intersphinx file (pydata#5665)
  Bump styfle/cancel-workflow-action from 0.9.0 to 0.9.1 (pydata#5663)
  pre-commit: autoupdate hook versions (pydata#5660)
  fix the binder environment (pydata#5650)
  Update api.rst (pydata#5639)
  Kwargs to rasterio open (pydata#5609)
  Bump codecov/codecov-action from 1 to 2.0.2 (pydata#5633)
  new blank whats-new for v0.19.1
  v0.19.0 release notes (pydata#5632)
  remove deprecations scheduled for 0.19 (pydata#5630)
  Make typing-extensions optional (pydata#5624)
  Plots get labels from pint arrays (pydata#5561)
  Add to_numpy() and as_numpy() methods (pydata#5568)
  pin fsspec (pydata#5627)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/main' into groupby-plot
7337ddd 
* upstream/main: (307 commits)
  Use same bool validator as other inputs (pydata#5703)
  conditionally disable bottleneck (pydata#5560)
  Refactor index vs. coordinate variable(s) (pydata#5636)
  pre-commit: autoupdate hook versions (pydata#5685)
  Flexible Indexes: Avoid len(index) in map_blocks (pydata#5670)
  Speed up _mapping_repr (pydata#5661)
  update the link to `scipy`'s intersphinx file (pydata#5665)
  Bump styfle/cancel-workflow-action from 0.9.0 to 0.9.1 (pydata#5663)
  pre-commit: autoupdate hook versions (pydata#5660)
  fix the binder environment (pydata#5650)
  Update api.rst (pydata#5639)
  Kwargs to rasterio open (pydata#5609)
  Bump codecov/codecov-action from 1 to 2.0.2 (pydata#5633)
  new blank whats-new for v0.19.1
  v0.19.0 release notes (pydata#5632)
  remove deprecations scheduled for 0.19 (pydata#5630)
  Make typing-extensions optional (pydata#5624)
  Plots get labels from pint arrays (pydata#5561)
  Add to_numpy() and as_numpy() methods (pydata#5568)
  pin fsspec (pydata#5627)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keewis keewis keewis left review comments

@dcherian dcherian dcherian left review comments

@shoyer shoyer Awaiting requested review from shoyer

@jhamman jhamman Awaiting requested review from jhamman

Assignees
No one assigned
Labels
plan to merge Final call for comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accessing COG overviews with read_rasterio
7 participants
@pkopparla @pep8speaks @max-sixty @keewis @dcherian @snowman2 @aurghs