added aggrgate_type argument in aggregate class method for SingleTime… #52
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 95.24% 95.02% -0.23%
==========================================
Files 33 33
Lines 2778 2815 +37
==========================================
+ Hits 2646 2675 +29
- Misses 132 140 +8 ☔ View full report in Codecov by Sentry. |
|
@daniel-thom or @pesap Could you review this ? |
| @@ -102,7 +102,7 @@ def check_data( | |||
| return data | |||
|
|
|||
| @classmethod | |||
| def aggregate(cls, ts_data: list[Self]) -> Self: | |||
| def aggregate(cls, ts_data: list[Self], agg_type: Literal["sum", "avg"] = "sum") -> Self: | |||
There was a problem hiding this comment.
I think we should explore to have this method as a standalone function. It feels unnatural to me that we define a method that can take multiple instances of itself. I am open for discussion.
| @@ -102,7 +102,7 @@ def check_data( | |||
| return data | |||
|
|
|||
| @classmethod | |||
| def aggregate(cls, ts_data: list[Self]) -> Self: | |||
| def aggregate(cls, ts_data: list[Self], agg_type: Literal["sum", "avg"] = "sum") -> Self: | |||
There was a problem hiding this comment.
I think the convention we have is to return "SingleTimeSeries" instead of self
There was a problem hiding this comment.
That's what I thought at first also. However, https://peps.python.org/pep-0673/#use-in-classmethod-signatures
| @@ -119,7 +119,7 @@ def aggregate(cls, ts_data: list[Self]) -> Self: | |||
| InconsistentTimeseriesAggregation | |||
| Raised if incompatible timeseries data are passed. | |||
| """ | |||
|
|
|||
| agg_func = {"sum": sum, "avg": lambda x: sum(x) / len(x)}[agg_type] | |||
There was a problem hiding this comment.
Peaky detail, but I am not a fam of defining functions inside functions. We can define this methods outside maybe?
There was a problem hiding this comment.
Also, wouldn't it be more consistent if we use numpy functions?
| if issubclass(magnitude_type, pa.Array): | ||
| new_data = sum( | ||
| if issubclass(magnitude_type, pa.Array) and is_quantity: | ||
| new_data = agg_func( |
There was a problem hiding this comment.
I think we could simply this cases. In all
the instances you have array and if it is pa.array we just need to convert it to numpy.
| @@ -264,6 +263,38 @@ def from_time_array( | |||
| def get_time_series_metadata_type() -> Type: | |||
| return SingleTimeSeriesMetadata | |||
|
|
|||
| # Support for scalar * SingleTimeSeries | |||
| def __mul__(self, scalar: int | float | BaseQuantity) -> Self: | |||
There was a problem hiding this comment.
I personally do not like this. Do we really need this?
…Series