Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

- A new class `Fuse` has been added to represent fuses. This class has a member variable `max_current` which represents the maximum current that can course through the fuse. If the current flowing through a fuse is greater than this limit, then the fuse will break the circuit.

- NaN values are treated as missing when gaps are determined in the `OrderedRingBuffer`.

## Bug Fixes

Expand Down
19 changes: 17 additions & 2 deletions src/frequenz/sdk/timeseries/_ringbuffer/buffer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Copy link
Contributor

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_value a function instead and using TypeGuard, but that would also need to take it back to operate on float | None:

from typing import TypeGuard

def has_value(sample: float | None) -> TypeGuard[float]:
    """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 value is not None and not value.isnan()

if has_value(sample):  # If this returns True, then the type checker narrows the type to `float`
    value = sample.value.base_value  # this is now fine without the assert
else:
    value = np.nan

I thought about alternatives and all seem awful, so I guess we'll have to live with the assert 🤷

value = sample.value.base_value
else:
value = np.nan
Comment on lines -157 to +172
Copy link
Contributor

Choose a reason for hiding this comment

The 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 np.nan anyway, or is it np.nan different from float('nan')?) without it too:

        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))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 infs at some point.)

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:
Expand Down
61 changes: 61 additions & 0 deletions tests/timeseries/test_ringbuffer.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,67 @@ def test_timestamp_ringbuffer_missing_parameter(
assert len(buffer.gaps) == 1


def dt(i: int) -> datetime: # pylint: disable=invalid-name
"""Create datetime objects from indices.

Args:
i: Index to create datetime from.

Returns:
Datetime object.
"""
return datetime.fromtimestamp(i, tz=timezone.utc)


def test_gaps() -> None:
"""Test gap treatment in ordered ring buffer."""
buffer = OrderedRingBuffer([0.0] * 5, ONE_SECOND)
assert len(buffer) == 0
assert len(buffer.gaps) == 0

buffer.update(Sample(dt(0), Quantity(0)))
assert len(buffer) == 1
assert len(buffer.gaps) == 1

buffer.update(Sample(dt(6), Quantity(0)))
assert len(buffer) == 1
assert len(buffer.gaps) == 1

buffer.update(Sample(dt(2), Quantity(2)))
buffer.update(Sample(dt(3), Quantity(3)))
buffer.update(Sample(dt(4), Quantity(4)))
assert len(buffer) == 4
assert len(buffer.gaps) == 1

buffer.update(Sample(dt(3), None))
assert len(buffer) == 3
assert len(buffer.gaps) == 2

buffer.update(Sample(dt(3), Quantity(np.nan)))
assert len(buffer) == 3
assert len(buffer.gaps) == 2

buffer.update(Sample(dt(2), Quantity(np.nan)))
assert len(buffer) == 2
assert len(buffer.gaps) == 2

buffer.update(Sample(dt(3), Quantity(3)))
assert len(buffer) == 3
assert len(buffer.gaps) == 2

buffer.update(Sample(dt(2), Quantity(2)))
assert len(buffer) == 4
assert len(buffer.gaps) == 1

buffer.update(Sample(dt(5), Quantity(5)))
assert len(buffer) == 5
assert len(buffer.gaps) == 0

buffer.update(Sample(dt(99), None))
assert len(buffer) == 4 # bug: should be 0 (whole range gap)
assert len(buffer.gaps) == 1


@pytest.mark.parametrize(
"buffer",
[
Expand Down