Skip to content

Conversation

@duy-ce
Copy link

@duy-ce duy-ce commented Jun 19, 2019

I think it should be quantile/2.0 first and then quantile in function add_quantile. In addition, in the result function you get dn[(marker_count - 1) / 2], so it must calculate to the position of quantile but in this code is quantile/2.0. Furthermore, this PR following the P^2 algorithm as documented in "The P-Square Algorithm for Dynamic Calculation of Percentiles and Histograms without Storing Observations," Communications of the ACM, October 1985 by R. Jain and I. Chlamtac.

Copy link
Owner

@absmall absmall left a comment

Choose a reason for hiding this comment

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

Hi, sorry for not noticing this, I don't monitor github much anymore.

This change will have no effect - the job of allocate_markers on line 84 is to increase the size of the dn array by 3, adding three more entries at the end. markers will point to the start of these three entries.

The markers shouldn't necessarily be at the end - they should be spread somehow throughout the existing markers depending on the order in which add_quantile is called for the various quantiles. So we can't possibly get the markers in the right order here, we don't even try, we just put the three markers in somehow.

Then on line 91, the call to update_markers sorts all the markers. So they should end up in the right order after that call

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