From 25f7938bd4f7f6e6d69792901ad8c3ae22299c1b Mon Sep 17 00:00:00 2001 From: javiber Date: Fri, 17 May 2024 12:37:02 -0300 Subject: [PATCH] Addressing changes requested --- temporian/core/event_set_ops.py | 2 +- temporian/core/operators/window/base.py | 9 ++++--- .../core/operators/window/moving_quantile.py | 26 ++++++++++++------- .../numpy_cc/operators/custom_heap.h | 26 ++++++------------- 4 files changed, 31 insertions(+), 32 deletions(-) diff --git a/temporian/core/event_set_ops.py b/temporian/core/event_set_ops.py index 4ca71eb9..4542bdd8 100644 --- a/temporian/core/event_set_ops.py +++ b/temporian/core/event_set_ops.py @@ -3254,7 +3254,7 @@ def moving_quantile( ... features={"value": [np.nan, 1, 5, 10, 15, 20]}, ... ) - >>> a.moving_quantile(tp.duration.seconds(4), quantile=0.5) + >>> a.moving_quantile(4, quantile=0.5) indexes: ... (6 events): timestamps: [0. 1. 2. 5. 6. 7.] diff --git a/temporian/core/operators/window/base.py b/temporian/core/operators/window/base.py index 6590898d..2d425fb9 100644 --- a/temporian/core/operators/window/base.py +++ b/temporian/core/operators/window/base.py @@ -34,8 +34,6 @@ class BaseWindowOperator(Operator, ABC): """Interface definition and common logic for window operators.""" - extra_attribute_def: List[Mapping[str, Any]] = [] - def __init__( self, input: EventSetNode, @@ -125,10 +123,15 @@ def has_variable_winlen(self) -> bool: def add_extra_attributes(self): pass + @classmethod + def extra_attribute_def(cls) -> List[Mapping[str, Any]]: + return [] + @classmethod def build_op_definition(cls) -> pb.OperatorDef: extra_attr_def = [ - pb.OperatorDef.Attribute(**attr) for attr in cls.extra_attribute_def + pb.OperatorDef.Attribute(**attr) + for attr in cls.extra_attribute_def() ] return pb.OperatorDef( key=cls.operator_def_key(), diff --git a/temporian/core/operators/window/moving_quantile.py b/temporian/core/operators/window/moving_quantile.py index caff068d..9cb302e1 100644 --- a/temporian/core/operators/window/moving_quantile.py +++ b/temporian/core/operators/window/moving_quantile.py @@ -14,7 +14,7 @@ """Moving count operator class and public API function definition.""" -from typing import Optional +from typing import List, Mapping, Optional, Any from temporian.core import operator_lib from temporian.core.compilation import compile @@ -27,14 +27,6 @@ class MovingQuantileOperator(BaseWindowOperator): - extra_attribute_def = [ - { - "key": "quantile", - "is_optional": True, - "type": pb.OperatorDef.Attribute.Type.FLOAT_64, - } - ] - def __init__( self, input: EventSetNode, @@ -47,9 +39,13 @@ def __init__( "`quantile` must be a float between 0 and 1. " f"Received {quantile}" ) - self.quantile = quantile + self._quantile = quantile super().__init__(input, window_length, sampling) + @property + def quantile(self) -> float: + return self._quantile + def add_extra_attributes(self): self.add_attribute("quantile", self.quantile) @@ -68,6 +64,16 @@ def get_feature_dtype(self, feature: FeatureSchema) -> DType: return DType.FLOAT32 return feature.dtype + @classmethod + def extra_attribute_def(cls) -> List[Mapping[str, Any]]: + return [ + { + "key": "quantile", + "is_optional": True, + "type": pb.OperatorDef.Attribute.Type.FLOAT_64, + } + ] + operator_lib.register_operator(MovingQuantileOperator) diff --git a/temporian/implementation/numpy_cc/operators/custom_heap.h b/temporian/implementation/numpy_cc/operators/custom_heap.h index 136a9fa6..9750af76 100644 --- a/temporian/implementation/numpy_cc/operators/custom_heap.h +++ b/temporian/implementation/numpy_cc/operators/custom_heap.h @@ -13,14 +13,16 @@ class CustomHeap { void push(T value) { heap.push_back(value); auto it = std::prev(heap.end()); + // Notice that this breaks if a value repeats, not a problem in our case + // since we are using the Heap to store the indices val_to_node[value] = it; - // TODO: better sorting? + // TODO: there is no better way to insert in order with a list + // but exploring with trees could make this better while (it != heap.begin()) { auto parent = std::prev(it); if (!compare(*parent, *it)) { break; } - // TODO: check that this swap is doing what I want std::swap(*parent, *it); val_to_node[*it] = it; val_to_node[*parent] = parent; @@ -35,6 +37,8 @@ class CustomHeap { auto value = heap.back(); heap.pop_back(); auto it = val_to_node.find(value); + // all other pointers in val_to_node are still valid because + // heap is a double linked list val_to_node.erase(it); return value; } @@ -52,25 +56,11 @@ class CustomHeap { auto it = val_to_node.find(value); if (it != val_to_node.end()) { heap.erase(it->second); + // all other pointers in val_to_node are still valid because + // heap is a double linked list val_to_node.erase(it); - } else { - // TODO: exception meant for debugging, remove it - throw std::invalid_argument("removing a value that doesn't exists"); } } int size() { return heap.size(); } int empty() { return heap.empty(); } - - void print() { - std::cout << "my_heap{" << std::endl << " ["; - std::for_each(heap.begin(), heap.end(), - [](const int n) { std::cout << n << ' '; }); - std::cout << "]" << std::endl; - - // std::cout << " {" << std::endl; - // for (const auto& pair : val_to_node) { - // std::cout << " " << pair.first << ": " << *(pair.second) << std::endl; - // } - // std::cout << " }" << std::endl; - } };