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

np.diagonal implementation #4431

Merged
merged 3 commits into from Feb 19, 2019

Conversation

Projects
None yet
6 participants
@horta
Copy link
Contributor

commented Jan 28, 2019

  • Tests added / passed
  • Passes flake8 dask

I couldn't make assert sorted(da.diagonal(v).dask) == sorted(da.diagonal(v).dask) work. The calls seems to be deterministic but it cannot sort the graph because tuple and str are not comparable. Shall I just remove it?

I would like to see da.diagonal implemented mainly because of da.trace, which will become easy to implement once this is done.

@horta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

Checklist: #2911

@mrocklin

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

cc @jakirkham , this may interest you

@mrocklin

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@shoyer or @jrbourbeau any interest in reviewing this?

@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Yeah, I'll have some time to look at this tomorrow 👍

@mrocklin

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@jrbourbeau
Copy link
Member

left a comment

Thanks for this PR @horta! I've attached a few comments and questions. In particular, I was wondering about the internal rechunk-ing that would happen for a large input Dask array

Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/tests/test_creation.py Outdated
Show resolved Hide resolved dask/array/tests/test_creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Checking in on this @horta . Is there any other information you need in order to move forward? It would be great to see this go in.

@horta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

No need for more information. I will get it going tomorrow!

@horta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

*working on that

Show resolved Hide resolved dask/array/creation.py Outdated
@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

This looks pretty good to me. There are a few comments above, but nothing too difficult I hope.

Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

This is coming along nicely, @horta. Will give it another look after you have some time to address the remaining comments. Probably one or two tidy up things in the next (last) iteration I think.

Show resolved Hide resolved dask/array/creation.py Outdated
Show resolved Hide resolved dask/array/creation.py Outdated
@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Thanks for the updates @horta. Added a couple comments above. Otherwise LGTM. @shoyer @mrocklin, any other thoughts?

@horta horta force-pushed the horta:diagonal branch from 09d3d67 to 4e1eecc Feb 14, 2019

@horta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Thanks, @jakirkham .

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

@jakirkham if you're comfortable with this PR then I encourage you to merge (I'm trying to get more people to become comfortable with pressing the green button).

@jcrist

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Thanks @horta, merging!

@jcrist jcrist merged commit faefaa8 into dask:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@mrocklin, as I'd requested feedback from others right before the 3-day weekend, figured I'd wait to see if I heard back this week to give people some time before clicking the merge button.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

No worries. Thanks for the encouragement.

@jakirkham

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Just out of curiosity, @horta, were you planning on implementing da.trace as well? Asking since you had mentioned it in the OP. :)

@horta

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@jakirkham sure, I aim to implement that by sunday

jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this pull request May 14, 2019

np.diagonal implementation (dask#4431)
* da.diagonal

* no need for asarray

* Direct asarray and use of np.diagonal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.