Support int indices and slice index behavior in ring buffer and moving window

[cwasicki]

To support int indices and slice index behavior in the moving window and the ring buffer, the following changes are made:

• Support for int indices in addition to datetime indices is added to moving window and ring buffer.
• Indices behave like usual slices (gracefully accept start >= end or index out of range).
• A method to count the number of possible elements in the range between oldest and newest timestamp is added to ring buffer and the moving window.
• get_timestamp is added to convert an int index passed by the user to the corresponding timestamp.

[llucax]

Some early feedback.

[llucax]

What if end is not a datetime but start is? Does it really works in that case or should this be

Suggested change

	if not (isinstance(start, datetime) and isinstance(end, datetime)):
	if not (isinstance(start, datetime) or not isinstance(end, datetime)):

Instead?

[cwasicki]

In the current case it only works if both are same type, if not, this one would throw. The idea was that either both are integer or both are datetime. Technically we could also support a mix of both, I was not sure whether we should. What do you think?

[llucax]

I would support the mix, unless there is a very strong case for it. My point above is, unless I'm understanding the code wrongly, the if above will not raise an error (at least not explicitly) if end is not a datetime but start is, right?

You have:

        # If both are indices or None convert to datetime
        if not isinstance(start, datetime) and not isinstance(end, datetime):
            start, end = self._to_covered_indices(start, end)
            start = self.get_timestamp(start)
            end = self.get_timestamp(end)

        # Here we should have both as datetime
        if not (isinstance(start, datetime) and isinstance(end, datetime)):
            raise IndexError(...)

Here you continue operating as if both start and end were datetimes,
So you should probably raise if any if start or end is not a datetime above.

[llucax]

Oh, sorry, I see I misread the parenthesis 🤦 (I was reading if not isinstance(start, datetime) and isinstance(end, datetime): (without the grouping of both and conditions to negate the result)

So scratch all the above and take this suggestion just as a code readability improvement 😆

Suggested change

	if not (isinstance(start, datetime) and isinstance(end, datetime)):
	if not isinstance(start, datetime) or not isinstance(end, datetime):

[cwasicki]

Updated according to your suggestion.

I would support the mix, unless there is a very strong case for it.

Could you clarify on this please, would you support a mix of datetime and int indices (e.g. moving_window[datetime(2023, 9, 25): -5]) or not? The current version does not, and honestly I find it confusing to use. We can still support it later if we find good use-cases.

[llucax]

Sorry, that was a typo, I meant "I would not support" 😞

[cwasicki]

Ok, that's what's currently implemented so this PR is ready. @llucax

[cwasicki]

@llucax Updated

[llucax]

So only one conversation left, and I was reading the code wrongly. As proven 😆 , I find the version without the extra parenthesis harder to read, but I guess every brain works differently, so completely optional.

[matthias-wende-frequenz]

This is very nice!

[shsms]

Reminder to remove "Still WIP" from description before merging

[cwasicki]

@llucax Any hint why the CI keeps failing?

[llucax]

I got it in other PRs too, I guess it is a change in pylint, but it is pinned, so it is very strange.

[llucax]

• Don't use generics unnecessarily #698