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.

Sign up for GitHub

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

Jump to bottom

feat: use a tree reduce for staged fills instead of pairwise adds #120

Merged
merged 2 commits into from
Jan 30, 2024
Merged

feat: use a tree reduce for staged fills instead of pairwise adds #120

merged 2 commits into from
Jan 30, 2024

Conversation

lgray
Copy link
Collaborator

@lgray lgray commented Jan 28, 2024 •
edited
Loading

This results in a task graph with better parallelism since now the leaves of the tree reduce do not wait on each other outside of split_every sized blocks. Better fault recovery too (fewer recomputed subgraphs if there's a failure).

@lgray lgray changed the title feat: use a tree reduce for stages fills instead of pairwise adds feat: use a tree reduce for staged fills instead of pairwise adds Jan 28, 2024
@lgray lgray force-pushed the tree_reduction_for_staged_fills branch 4 times, most recently from da89736 to 0efc799 Compare January 28, 2024 22:50
@martindurant
Copy link
Collaborator

This results in a task graph with better parallelism

For posterity, can we have images of before and after?

@lgray lgray force-pushed the tree_reduction_for_staged_fills branch from 0efc799 to 235e4d1 Compare January 29, 2024 19:55
@lgray
Copy link
Collaborator Author

lgray commented Jan 29, 2024

Here's before:
image

Here's after:
image

martindurant
martindurant reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@martindurant martindurant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before going further, please consider whether to use the upstream dask tree aggregation maker

src/dask_histogram/boost.py Outdated Show resolved Hide resolved
src/dask_histogram/boost.py Show resolved Hide resolved
src/dask_histogram/boost.py Show resolved Hide resolved
src/dask_histogram/boost.py Outdated Show resolved Hide resolved
src/dask_histogram/boost.py Outdated Show resolved Hide resolved
src/dask_histogram/core.py Show resolved Hide resolved
src/dask_histogram/core.py Show resolved Hide resolved
@martindurant martindurant merged commit 5289f10 into dask-contrib:main Jan 30, 2024
18 checks passed
@lgray lgray deleted the tree_reduction_for_staged_fills branch January 30, 2024 04:02
lgray added a commit to lgray/dask-histogram that referenced this pull request Feb 11, 2024
@lgray
Revert "Merge pull request dask-contrib#120 from lgray/tree_reduction…
c33fe0b 
…_for_staged_fills"

This reverts commit 5289f10, reversing
changes made to 6e3fa1a.
@ikrommyd ikrommyd mentioned this pull request Feb 23, 2024
Merged
lgray added a commit to lgray/dask-histogram that referenced this pull request Mar 1, 2024
@lgray
Revert "Merge pull request dask-contrib#120 from lgray/tree_reduction…
5624977 
…_for_staged_fills"

This reverts commit 5289f10, reversing
changes made to 6e3fa1a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martindurant martindurant martindurant 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.
2 participants
@lgray @martindurant