-
Notifications
You must be signed in to change notification settings - Fork 20
Treat NaN as gap in ring buffer #641
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,17 @@ def gaps(self) -> List[Gap]: | |
| """ | ||
| return self._gaps | ||
|
|
||
| def has_value(self, sample: Sample[QuantityT]) -> bool: | ||
| """Check if a sample has a value and it's not NaN. | ||
|
|
||
| Args: | ||
| sample: sample to check. | ||
|
|
||
| Returns: | ||
| True if the sample has a value and it's not NaN. | ||
| """ | ||
| return not (sample.value is None or sample.value.isnan()) | ||
|
|
||
| @property | ||
| def maxlen(self) -> int: | ||
| """Get the max length. | ||
|
|
@@ -154,10 +165,14 @@ def update(self, sample: Sample[QuantityT]) -> None: | |
| ) | ||
|
|
||
| # Update data | ||
| value: float = np.nan if sample.value is None else sample.value.base_value | ||
| if self.has_value(sample): | ||
| assert sample.value is not None | ||
| value = sample.value.base_value | ||
| else: | ||
| value = np.nan | ||
|
Comment on lines
-157
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or.... ❗❗❗ just not use the function in this context... The original code is pretty simple and understandable (and works the same, as if it is NaN we'll use valueL = np.nan if sample.value is None else sample.value
self._buffer[self.datetime_to_index(timestamp)] = value
self._update_gaps(timestamp, prev_newest, not self.has_value(sample))There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to leave it as it is required in both these places. So you have one location where you define what a missing value is and what not (e.g. if we want to change it to include Actually I find it not so bad, even more readable. But yea, the assertion is annoying. |
||
| self._buffer[self.datetime_to_index(timestamp)] = value | ||
|
|
||
| self._update_gaps(timestamp, prev_newest, sample.value is None) | ||
| self._update_gaps(timestamp, prev_newest, not self.has_value(sample)) | ||
|
|
||
| @property | ||
| def time_bound_oldest(self) -> datetime: | ||
|
|
||
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.
I just noticed this, too bad that this is needed, because it was just checked by
has_value().You could avoid this by making
has_valuea function instead and usingTypeGuard, but that would also need to take it back to operate onfloat | None:I thought about alternatives and all seem awful, so I guess we'll have to live with the assert 🤷