[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #322: Python wrapper improvement

[GitBox <gi...@apache.org>]

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


##########
python/src/quantiles_wrapper.cpp:
##########
@@ -114,12 +34,27 @@ void bind_quantiles_sketch(py::module &m, const char* name) {
   py::class_<quantiles_sketch<T>>(m, name)
     .def(py::init<uint16_t>(), py::arg("k")=quantiles_constants::DEFAULT_K)
     .def(py::init<const quantiles_sketch<T>&>())
-    .def("update", (void (quantiles_sketch<T>::*)(const T&)) &quantiles_sketch<T>::update, py::arg("item"),
-         "Updates the sketch with the given value")
-    .def("update", &dspy::quantiles_sketch_update<T>, py::arg("array"),
-         "Updates the sketch with the values in the given array")
+    .def(
+        "update",

Review Comment:
   By the time we have conditionals and exceptions, I feel like encapsulating the logic in a function elsewhere is probably a win for interface readability.



##########
python/tests/vo_test.py:
##########
@@ -45,6 +45,13 @@ def test_vo_example(self):
     items = vo.get_samples()
     self.assertEqual(len(items), k)
 
+    count = 0
+    for tuple in vo:
+      sample = tuple[0]
+      weight = tuple[1]

Review Comment:
   worth summing weights and ensuring they equal the total?



##########
python/src/fi_wrapper.cpp:
##########
@@ -105,15 +53,55 @@ void bind_fi_sketch(py::module &m, const char* name) {
          "Returns the guaranteed upper bound weight (frequency) of the given item.")
     .def("get_sketch_epsilon", (double (frequent_items_sketch<T>::*)(void) const) &frequent_items_sketch<T>::get_epsilon,
          "Returns the epsilon value used by the sketch to compute error")
-    .def_static("get_epsilon_for_lg_size", &dspy::fi_sketch_get_generic_epsilon<T>, py::arg("lg_max_map_size"),
-         "Returns the epsilon value used to compute a priori error for a given log2(max_map_size)")
-    .def_static("get_apriori_error", &frequent_items_sketch<T>::get_apriori_error, py::arg("lg_max_map_size"), py::arg("estimated_total_weight"),
-         "Returns the estimated a priori error given the max_map_size for the sketch and the estimated_total_stream_weight.")
-    .def("get_serialized_size_bytes", &dspy::fi_sketch_get_serialized_size_bytes<T>,
-         "Computes the size needed to serialize the current state of the sketch. This can be expensive since every item needs to be looked at.")
-    .def("serialize", &dspy::fi_sketch_serialize<T>, "Serializes the sketch into a bytes object")
-    .def_static("deserialize", &dspy::fi_sketch_deserialize<T>, "Reads a bytes object and returns the corresponding frequent_strings_sketch")
-    ;
+    .def(

Review Comment:
   I'm not actually convinced that this long a lambda is an improvement to readability.



-- 
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