-
Notifications
You must be signed in to change notification settings - Fork 681
feat: support csc in dask arrays in get.aggregate
#3872
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3872 +/- ##
=======================================
Coverage 76.76% 76.77%
=======================================
Files 115 115
Lines 12365 12370 +5
=======================================
+ Hits 9492 9497 +5
Misses 2873 2873
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As usual, just some stylistic stuff. Great work!
I’m not super happy about the added complexity, but I also don’t know if we could do it better:
e.g. it doesn’t look like we can pull out everything depending on {un,}chunked_axis and pass that in from a function that just exists to handle that complexity, things are too interwoven. What do you think?
Co-authored-by: Philipp A. <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Philipp A. <[email protected]>
Co-authored-by: Philipp A. <[email protected]>
|
what do you say to what I wrote in #3872 (review)? |
Ah yeah, I meant to reply to that. I can look into it. Let's leave this PR as-is. I'll open a new one into this one fiddling around with it. If it looks to be too complex, I may just punt and say "FAU when we can" |
sc.get.aggregatewith CSC dask #3861