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

jgrss/store #189

Merged
merged 56 commits into from
Sep 1, 2022
Merged

jgrss/store #189

merged 56 commits into from
Sep 1, 2022

Conversation

jgrss
Copy link
Owner

@jgrss jgrss commented Aug 31, 2022

What is this PR changing?

This PR adds:

  • A new save() method
  • Delayed returns in place of in-memory numpy arrays
  • Better CRS checks
  • More tests

@jgrss jgrss requested a review from mmann1123 August 31, 2022 22:31
@jgrss
Copy link
Owner Author

jgrss commented Aug 31, 2022

@mmann1123 No rush on this, but I think this is getting close.

@mmann1123 mmann1123 marked this pull request as ready for review September 1, 2022 13:02
@jgrss
Copy link
Owner Author

jgrss commented Sep 1, 2022

@mmann1123 FYI, ML tests are turned off because they are failing and I haven't had time to look at them.

@mmann1123
Copy link
Collaborator

@jgrss I am throwing an error on install

pip install git+https://github.com/jgrss/geowombat.git@jgrss/store

ERROR: Could not find a version that satisfies the requirement xarray>=2022.6.0 (from geowombat==1.11.4) (from versions: 0.7.0, 0.7.1, 0.7.2, 0.8.0rc1, 0.8.0, 0.8.1, 0.8.2, 0.9.0rc1, 0.9.0, 0.9.1, 0.9.2, 0.9.3, 0.9.4, 0.9.5, 0.9.6, 0.10.0rc1, 0.10.0rc2, 0.10.0, 0.10.1, 0.10.2, 0.10.3, 0.10.4, 0.10.5, 0.10.6, 0.10.7, 0.10.8, 0.10.9, 0.11.0, 0.11.1, 0.11.2, 0.11.3, 0.12.0, 0.12.1, 0.12.2, 0.12.3, 0.13.0, 0.14.0, 0.14.1, 0.15.0, 0.15.1, 0.16.0, 0.16.1, 0.16.2, 0.17.0, 0.18.0, 0.18.1, 0.18.2, 0.19.0, 0.20.0, 0.20.1, 0.20.2)
ERROR: No matching distribution found for xarray>=2022.6.0 (from geowombat==1.11.4)

@mmann1123
Copy link
Collaborator

@jgrss I am going to try in docker - maybe just something on my end.

@mmann1123
Copy link
Collaborator

@jgrss Ok installed fine on docker and seems to be running smoothly. I did run into an issue with h5netcdf not being available for testing but I'll leave it to you as to whether we include it in requirements

Copy link
Collaborator

@mmann1123 mmann1123 left a comment

Choose a reason for hiding this comment

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

Just a few minor comments I'll let you squash and merge @jgrss

@@ -408,21 +430,21 @@ def mosaic(

darray = xr.where((darray.mean(dim='band') == nodata) & (darrayb.mean(dim='band') != nodata), darrayb,
xr.where((darray.mean(dim='band') != nodata) & (darrayb.mean(dim='band') == nodata), darray,
xr_mininum(darray, darrayb)))
np.mininum(darray, darrayb)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok good to see this... maybe this resolves the conda issue

Args:
filename (str | Path): The output file name to write to.
overwrite (Optional[bool]): Whether to overwrite an existing file. Default is False.
client (Optional[Client object]): A client object to persist data. Default is None.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a reference to a dask client?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, I updated this docstring.

filename (str | Path): The output file name to write to.
overwrite (Optional[bool]): Whether to overwrite an existing file. Default is False.
client (Optional[Client object]): A client object to persist data. Default is None.
compute (Optinoal[bool]): Whether to compute and write to ``filename``. Otherwise, return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be explicit
compute (Optinoal[bool]): If True, compute and write to ``filename``. If False, return the ``dask`` task graph. Default is ``True``.

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

Successfully merging this pull request may close these issues.

None yet

2 participants