Skip to content

Make argmin/max work lazy with dask - #3244

Merged
dcherian merged 8 commits into
pydata:masterfrom
ulijh:lazy_argmax_min
Sep 6, 2019
Merged

Make argmin/max work lazy with dask#3244
dcherian merged 8 commits into
pydata:masterfrom
ulijh:lazy_argmax_min

Conversation

@ulijh

@ulijhulijh commented Aug 22, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

As @shoyer pointed out in #3237, nanargmax/min from numpy or dask should be used when not working on object arrays. Also, nanargmin/max were added to the nputils module so they should be using bottleneck if available.

@ulijhulijh changed the title Make argmin/max work lazy with dask (#3237).Make argmin/max work lazy with daskAug 22, 2019
@ulijhulijh mentioned this pull request Aug 22, 2019
Closed
@max-sixty

max-sixty commented Aug 22, 2019

Copy link
Copy Markdown
Collaborator

This looks good @ulijh ! Thanks

Could you add a test? Also feel free to give yourself credit in a whatsnew

@ulijh

ulijh commented Aug 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks @max-sixty! Sure, I'll do this tomorrow then.

@ulijh

ulijh commented Aug 23, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Guys, if you could have a look at the tests I modified (a4c3622) to check how many times things get computed. I tried to integrate it with the existing tests. The number of count check could possibly be applied to some other tests as well. May be there is a smarter way of doing this?

shoyer
shoyer reviewed Aug 26, 2019
View reviewed changes
Comment threadxarray/tests/test_dask.py Outdated
Comment threadxarray/tests/test_dask.py Outdated
Comment threadxarray/tests/test_dask.py Outdated
Comment threadxarray/tests/test_dask.py
Comment threadxarray/core/nanops.py Outdated
ulijhand others added 4 commits August 27, 2019 13:48
@ulijh@shoyer
Fix typo
1b4bc3a
Co-Authored-By: Stephan Hoyer <shoyer@gmail.com>
@ulijh@shoyer
Be more explicit.
abe6655
Co-Authored-By: Stephan Hoyer <shoyer@gmail.com>
Ulrich J. Herter
More explicit raise_if_dask_computes
c0d7462
Ulrich J. Herter
nanargmin/max: only set fill_value when needed
faa7eab
@ulijh

ulijh commented Aug 27, 2019

Copy link
Copy Markdown
ContributorAuthor

May be this is a stupid question: What's the best way to resolve this conflict, and get the checks to run? Should I do a merge master? Or rebase (seems somewhat of a pain to get the remote up to date)? Thanks for the advice!

@dcherian

dcherian commented Aug 27, 2019

Copy link
Copy Markdown
Contributor

Merge master is enough. I just did it to kick off the tests.

@shoyer

shoyer commented Aug 27, 2019 via email

Copy link
Copy Markdown
Member

@ulijh

ulijh commented Aug 27, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks guys!

@dcherian
dcherian merged commit 0a046db into pydata:masterSep 6, 2019
dcherian added a commit to dcherian/xarray that referenced this pull request Sep 19, 2019
@dcherian
Merge branch 'master' into map_blocks_2
599b70a
* master:
Fix whats-new date :/
Revert to dev version
Release v0.13.0
auto_combine deprecation to 0.14 (pydata#3314)
Deprecation: groupby, resample default dim. (pydata#3313)
Raise error if cmap is list of colors (pydata#3310)
Refactor concat to use merge for non-concatenated variables (pydata#3239)
Honor `keep_attrs` in DataArray.quantile (pydata#3305)
Fix DataArray api doc (pydata#3309)
Accept int value in head, thin and tail (pydata#3298)
ignore h5py 2.10.0 warnings and fix invalid_netcdf warning test. (pydata#3301)
Update why-xarray.rst with clearer expression (pydata#3307)
Compat and encoding deprecation to 0.14 (pydata#3294)
Remove deprecated concat kwargs. (pydata#3288)
allow np-array levels and colors in 2D plots (pydata#3295)
Remove some deprecations (pydata#3292)
Make argmin/max work lazy with dask (pydata#3244)
Add head, tail and thin methods (pydata#3278)
Updater to testing environment name (pydata#3253)
@ulijh
ulijh deleted the lazy_argmax_min branch October 25, 2019 11:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shoyershoyershoyer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

argmax() causes dask to compute

4 participants

@ulijh@max-sixty@dcherian@shoyer