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

Associative Reduce #129

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Associative Reduce #129

merged 4 commits into from
Feb 27, 2024

Conversation

srush
Copy link
Collaborator

@srush srush commented Feb 25, 2024

Hey @danoneata! Long time!

I've been using chalk for a new project and I realized that functools is really dumb about reductions. It doesn't make any assumption about the associativity of the operator which means we are recursing to an O(N) depth with the functional style not O(log(N)). This change fixes the problem with no change to the underlying code.

@danoneata
Copy link
Collaborator

Hello Sasha! I'm very happy to hear from you! I've noticed that you are still making good use of chalk in your side projects. I feel a bit guilty that I couldn't keep up with the active development of the library, but I still hope to find some free time to start addressing some of the issues.

Thanks for the PR! Yes, efficiency is a sore point of the current implementation, so using the log(N) variant of reduce is a good step in the right direction.

@danoneata danoneata merged commit 8d8feab into master Feb 27, 2024
0 of 4 checks passed
@srush
Copy link
Collaborator Author

srush commented Feb 27, 2024

Honestly it mostly works well for my use cases! Speed is mostly not a problem.

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.

2 participants