-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adding ckmeans #36
Comments
@neocarto any thoughts on this ? |
Totally agree to add this method (in addition to jenks) |
There is already an implementation in simple-statistics. Not that simple statistics is particularly heavy, but I'm not sure we should depend on it just for this method. The code for simple statistics is licensed under the ISC licence, which is relatively permissive but requires (I think) that we retain the original licence notice and the author's name. I think we can include / adapt the simple statistics code with the following notice (in our futur ckmeans code):
If we do so, I also think it might be a good idea to indicate in the README that the "ckmeans" method comes from Tom MacWright / simple statistics. Or maybe with something like this in the LICENSE file :
What do you think ? |
It's possible that it's faster than our Fisher-Jenks implementation (or not worse).
And our users might need it (moreover other libs like simple statistics or bin guru are offering it).
See also https://observablehq.com/@visionscarto/natural-breaks that is advocating for the use of this method (in replacement of Jenks - but I think we should still have Jenks in
statsbreaks
)The text was updated successfully, but these errors were encountered: