-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added aggregate class method on SingleTimeSeries class #48
Conversation
@daniel-thom Could you review this ? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
=======================================
Coverage ? 95.24%
=======================================
Files ? 33
Lines ? 2778
Branches ? 0
=======================================
Hits ? 2646
Misses ? 132
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just two minor comments.
@@ -89,6 +101,72 @@ def check_data( | |||
|
|||
return data | |||
|
|||
@classmethod | |||
def aggregate(cls, ts_data: list[Self]) -> Self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps [Iterable]
instead of list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually no mypy complains if I do iterable because I am accessing ts_data in multiple places which you can not do if you pass generator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, because you are accessing it by index. Technically, you could have made it typing.Sequence
, which allows list or tuple, at least. But don't worry about it.
src/infrasys/time_series_models.py
Outdated
|
||
# Validate uniformity across properties | ||
if any(len(prop) != 1 for prop in unique_props.values()): | ||
msg = f"Inconsistent timeseries data: {unique_props}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should only print the sets with length greater than one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
added aggregate class method on SingleTimeSeries class
No description provided.