Tdigest #418
Tdigest #418
Conversation
Pull Request Test Coverage Report for Build 7703245801
💛 - Coveralls |
There was a problem hiding this comment.
A few minor comments.
Overall, I can mostly follow what it's doing but not necessarily why without consulting the original paper (which I really should be but haven't). The scale function in particular and how it's used makes the least sense.
Anyway, approving but there's at least one place where a comment would be quite helpful in thw future.
tdigest/CMakeLists.txt
Outdated
|
|
||
| add_library(tdigest INTERFACE) | ||
|
|
||
| add_library(${PROJECT_NAME}::QUANTILES ALIAS tdigest) |
There was a problem hiding this comment.
ops, will fix
| REQUIRE(td.get_quantile(0.5) == deserialized_td.get_quantile(0.5)); | ||
| } | ||
|
|
||
| TEST_CASE("serialize deserialize steam and bytes equivalence", "[tdigest]") { |
There was a problem hiding this comment.
I think we've not done a robust job of testing this explicitly elsewhere, so I appreciated this.
| static const uint8_t PREAMBLE_LONGS_EMPTY = 1; | ||
| static const uint8_t PREAMBLE_LONGS_NON_EMPTY = 2; | ||
| static const uint8_t SERIAL_VERSION = 1; | ||
| static const uint8_t SKETCH_TYPE = 20; |
There was a problem hiding this comment.
don't forget to reserve this in java's Family.java
| const double proposed_weight = centroids_.back().get_weight() + it->get_weight(); | ||
| const double projected_weight = weight_so_far + proposed_weight; | ||
| bool add_this; | ||
| if (USE_WEIGHT_LIMIT) { |
There was a problem hiding this comment.
There may be a good reason, but this code reads like "if USE_WEIGHT_LIMIT then don't use the w_limit value" which is a little odd. Could probably use a comment.
See #409