Skip to content

[Suggestion] Small exports for distributed programs#3

Open
SGrondin wants to merge 2 commits intowelch:masterfrom
SGrondin:master
Open

[Suggestion] Small exports for distributed programs#3
SGrondin wants to merge 2 commits intowelch:masterfrom
SGrondin:master

Conversation

@SGrondin
Copy link
Copy Markdown

Hi,

This is a fork used in large distributed programs where I work. It adds a Distributable class that inherits from Digest. The purpose of that class is to minimize the size of the exported state (toArray) so that a node wanting to read a percentile value can fetch lots of small internal states from each node and recompute the percentile quickly.

It implements toList(), which is a more compact version of toArray(). It uses arrays to save space on the countless mean: ..., n: .... The centroids can be pushed back into a new Distributable instance using .push(centroid[0], centroid[1]).

I have no idea if this would be useful to you or anyone else, but I'm opening this PR in case you find it interesting and/or want to merge it.

@welch
Copy link
Copy Markdown
Owner

welch commented Feb 29, 2016

Thanks! I'll give it a look later today.

@SGrondin
Copy link
Copy Markdown
Author

SGrondin commented Mar 1, 2016

To be honest, I think it's a very narrow use case, the settings are hardcoded and there's no tests. I would be surprised if you merged is as is. I opened the PR because if I do work on top of open source software I like to show the author how it's being used in case it gives them ideas :)

@welch
Copy link
Copy Markdown
Owner

welch commented Mar 2, 2016

As you say, it's not mergeable code (I'd hit you up for unit tests at least).
But it's a pretty classy way to submit a feature request 😄

I'll take this on so I can get you back to running the main line.

Thanks!

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