Re: [PR] uint32_t weight in tdigest + cross-language test [datasketches-cpp]

["jmalkin (via GitHub)" <gi...@apache.org>]

jmalkin commented on code in PR #424:
URL: https://github.com/apache/datasketches-cpp/pull/424#discussion_r1502151245


##########
tdigest/include/tdigest.hpp:
##########
@@ -84,18 +85,20 @@ class tdigest {
   static const bool USE_TWO_LEVEL_COMPRESSION = true;
   static const bool USE_WEIGHT_LIMIT = true;
 
+  using W = typename std::conditional<std::is_same<T, double>::value, uint64_t, uint32_t>::type;
+
   class centroid {
   public:
-    centroid(T value, uint64_t weight): mean_(value), weight_(weight) {}
+    centroid(T value, W weight): mean_(value), weight_(weight) {}
     void add(const centroid& other) {
       weight_ += other.weight_;
       mean_ += (other.mean_ - mean_) * other.weight_ / weight_;
     }
     T get_mean() const { return mean_; }
-    uint64_t get_weight() const { return weight_; }
+    T get_weight() const { return weight_; }

Review Comment:
   return type W?



##########
tdigest/test/tdigest_test.cpp:
##########
@@ -204,6 +204,27 @@ TEST_CASE("serialize deserialize bytes non empty", "[tdigest]") {
   REQUIRE(td.get_quantile(0.5) == deserialized_td.get_quantile(0.5));
 }
 
+TEST_CASE("serialize deserialize steam and bytes equivalence empty", "[tdigest]") {
+  tdigest<double> td(100);
+  std::stringstream s(std::ios::in | std::ios::out | std::ios::binary);
+  td.serialize(s);
+  auto bytes = td.serialize();
+
+  REQUIRE(bytes.size() == static_cast<size_t>(s.tellp()));
+  for (size_t i = 0; i < bytes.size(); ++i) {
+    REQUIRE(((char*)bytes.data())[i] == (char)s.get());
+  }
+
+  s.seekg(0); // rewind
+  auto deserialized_td1 = tdigest<double>::deserialize(s);
+  auto deserialized_td2 = tdigest<double>::deserialize(bytes.data(), bytes.size());
+  REQUIRE(bytes.size() == static_cast<size_t>(s.tellg()));
+
+  REQUIRE(deserialized_td1.get_k() == deserialized_td2.get_k());
+  REQUIRE(deserialized_td1.get_total_weight() == deserialized_td2.get_total_weight());

Review Comment:
   This never actually checks that the weight is 0 or that `is_empty()` returns true, only that they're equivalent



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org