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-unsubscr...@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@datasketches.apache.org
For additional commands, e-mail: commits-h...@datasketches.apache.org
