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

[WIP] Add k arg to diag #5683

Closed
wants to merge 2 commits into from
Closed

Conversation

TAdeJong
Copy link
Contributor

@TAdeJong TAdeJong commented Dec 5, 2019

This closes #2726 and reduces some code duplication by calling da.diagonal where applicable. This for now breaks by #5661, therefore the WIP, although I don't feel equipped or have the time to solve that issue here.

  • Tests added / passed
  • Passes black dask / flake8 dask

Seems to work now, cleaned out comments.
da.diagonal remains broken however.
@TomAugspurger TomAugspurger added this to In progress in Core maintenance Feb 4, 2020
@TomAugspurger
Copy link
Member

Sorry for the delay in review.

This for now breaks by #5661

Can you expand on that? You're saying #5661 is a blocker for fixing this? I'm looking into that a bit now.

@TAdeJong
Copy link
Contributor Author

TAdeJong commented Feb 6, 2020

This reuses code from da.diagonal in da.diag. The test that fails here, fails because of the issue described in #5661, as test_corrcoef() calls da.diag, which since this pull request calls da.diagonal, exposing the bug described in #5661. In principle this code can be merged, except that the test now fails because of #5661.

@TomAugspurger is that clear? Please let me know if something is still unclear

@TomAugspurger
Copy link
Member

TomAugspurger commented Feb 10, 2020 via email

@mrocklin mrocklin moved this from In progress to Backlog in Core maintenance Feb 11, 2020
@mrocklin mrocklin moved this from Backlog to Sorted Backlog in Core maintenance Feb 21, 2020
@martindurant
Copy link
Member

@TomAugspurger , were you working on ironing things out here?

@TomAugspurger
Copy link
Member

TomAugspurger commented Apr 7, 2020 via email

@TAdeJong
Copy link
Contributor Author

TAdeJong commented Apr 7, 2020

For the overview: the other issue would be #5661 , which @jsignell seems to be working on. By the time that is resolved, I would be happy to bring this one into mergable shape.

@jcrist
Copy link
Member

jcrist commented Feb 23, 2022

Superseded by #8689.

@jcrist jcrist closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Core maintenance
Sorted Backlog
Development

Successfully merging this pull request may close these issues.

Dask Array's diag supporting diagonal selection
4 participants