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

Use _unstack_once for valid dask and sparse versions #5315

Merged
merged 26 commits into from
May 17, 2021

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented May 15, 2021

@pep8speaks
Copy link

pep8speaks commented May 15, 2021

Hello @Illviljan! 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-05-17 21:56:42 UTC

@Illviljan Illviljan marked this pull request as draft May 15, 2021 11:51
@Illviljan Illviljan changed the title Faster unstacking with dask index assignment Use _unstack_once for valid dask and sparse versions May 15, 2021
xarray/core/dataset.py Outdated Show resolved Hide resolved
@Illviljan
Copy link
Contributor Author

pint doesn't seem to be playing nice still @max-sixty. Seems some of your issues from #4746 are still there? Eventhough some of the issues in #4751 seems to have been solved.

@max-sixty
Copy link
Collaborator

pint doesn't seem to be playing nice still @max-sixty. Seems some of your issues from #4746 are still there? Eventhough some of the issues in #4751 seems to have been solved.

@keewis any thoughts?

One (kinda bad) idea would be to branch on whether pint was installed — presumably if it's not installed then it can't cause the import issues. But changing dask performance based on whether pint is installed is not great!

@keewis
Copy link
Collaborator

keewis commented May 16, 2021

yep, to really fix this we would have to change pint. I think it would be worth verifying whether we can check the interface (with hasattr) instead of type checking using isinstance, though.

@max-sixty
Copy link
Collaborator

yep, to really fix this we would have to change pint. I think it would be worth verifying whether we can check the interface (with hasattr) instead of type checking using isinstance, though.

Great, that could be a good workaround. What attr would we check for?

@Illviljan Illviljan closed this May 16, 2021
@Illviljan Illviljan reopened this May 16, 2021
@keewis keewis closed this May 16, 2021
@keewis keewis reopened this May 16, 2021
@Illviljan
Copy link
Contributor Author

@keewis any idea what it means when only 1 check has been run?

@keewis
Copy link
Collaborator

keewis commented May 16, 2021

github actions currently has "degraded performance" (earlier today it was a "major outage")

@max-sixty
Copy link
Collaborator

Looks great! Let's merge on green. Feel free to add a whatsnew @Illviljan !

@Illviljan Illviljan marked this pull request as ready for review May 17, 2021 21:36
@Illviljan Illviljan closed this May 17, 2021
@Illviljan Illviljan reopened this May 17, 2021
@max-sixty
Copy link
Collaborator

Great @Illviljan ! Feel free to add a whatsnew in a different PR — this is good news for lots of dask users

@max-sixty max-sixty merged commit 9165c26 into pydata:master May 17, 2021
@Illviljan Illviljan deleted the Illviljan-faster_unstacking branch May 18, 2021 18:13
max-sixty added a commit to max-sixty/xarray that referenced this pull request May 19, 2021
max-sixty added a commit that referenced this pull request May 19, 2021
* Revert "Use _unstack_once for valid dask and sparse versions (#5315)"

This reverts commit 9165c26.

* 0.18.2 release notes
@Illviljan Illviljan mentioned this pull request Jul 5, 2021
3 tasks
TomNicholas added a commit that referenced this pull request Jul 23, 2021
* Revert "Use _unstack_once for valid dask and sparse versions (#5315)"

This reverts commit 9165c26.

* 0.18.2 release notes

* fix RTD [skip-ci] (#5518)

* v0.19.0 release notes

* Update doc/whats-new.rst [skip-ci]

* remove empty sections

Co-authored-by: Maximilian Roos <[email protected]>
Co-authored-by: keewis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants