Change resolution to resample_period

[flora-hofmann-frequenz]

Addressing Issue #79.

[cwasicki]

This can also be None if no resampling is desired.

[flora-hofmann-frequenz]

I get a mypy error when I set it to None and the use total_seconds() in the streaming filter. Not quite sure how to work around it?

[flora-hofmann-frequenz]

_client.py:303: error: Item "None" of "timedelta | None" has no attribute "total_seconds" [union-attr]

[cwasicki]

If it's None you can pass that to the API, otherwise you convert it. E.g.

resolution = td.total_seconds() if sp is not None else None

[flora-hofmann-frequenz]

I have added this in.

[cwasicki]

Just noting that the SDK uses sampling_period, but I think the term you chose is more accurate, so I would keep it.

[Marenz]

I am wondering, do we want int() which cuts off the fractional part or rather round() which .. well.. rounds ;)

[flora-hofmann-frequenz]

I am not sure, but we had resolution : int beforehand, that's why I kept it this way

[flora-hofmann-frequenz]

But it also makes sense, cause the client/user would likely use 1min, 15min and probably nothing too complicated in between. If it needs to be as it is read out then one can just leave it defaulting to None.

[Marenz]

sure, but due to floating point, you could end up with an number like 4.999999 in there and instead of 5 you'll get 4 this way.

[flora-hofmann-frequenz]

But if the proto defines uint32 should we then not also use int here?

[cwasicki]

You would still use int because the API doesn't support sub-second resolution. But int just cuts off the fractional part, so what @Marenz describes is a problem with floating points in general where you cannot always represent integers exactly. So I agree with rounding here.

[cwasicki]

where you cannot always represent integers exactly.

In practice this should less of a concern in the range we use this parameter, maybe more if the value is a result of floating point operations I guess. But it's still good practice :)

[Marenz]

Is anywhere explained what unit this parameter expects? And is it desired that sub-second durations (if those are seconds) are not supported (int)?

[flora-hofmann-frequenz]

This is the reference to the proto file: https://github.com/frequenz-floss/frequenz-api-reporting/blob/72510043a64517dd0f7eba1cd6ea8b0fa6f898ae/proto/frequenz/api/reporting/v1/reporting.proto#L81

[Marenz]

Right, but when calling this program with "--help", I won't have the proto buf in mind and how something maps exactly from that to my command line parameter?

[flora-hofmann-frequenz]

ahhh sorry, you mean i should add something like: help="Resampling period in seconds (integer, no sub-second durations)." (obviously pending our discussion above).

[Marenz]

The parameter here is int, so that "sub second" part is not needed. But you convert it later to a time-delta which then converts it to seconds, but in floating point. And that last conversion is only the part where the problem can happen. Though rather theoretical maybe, depending what the source of the value is :)

[Marenz]

No need to change it now, but for future, I think the extra type info isn't required in the comment, the type hints already give us that :)

[flora-hofmann-frequenz]

Good point! I have taken it out since I had to rebase on v0.x.x to get rid of the merge commit in any case.

I just saw a merge commit?

[Marenz]

🎆

[cwasicki]

You are passing an int (and in fact also None is possible, which is missing here). I would leave this and convert to timedelta (if set) below.

[flora-hofmann-frequenz]

Sorry, I am not entirely sure that I understand this comment. I tried the following change:

resampling_period: int | None,
states: bool,
bounds: bool,
service_address: str,
key: str,
fmt: str,

) -> None:
"""Test the ReportingApiClient.

Args:
    microgrid_id: microgrid ID
    component_id: component ID
    metric_names: list of metric names
    start_dt: start datetime
    end_dt: end datetime
    resampling_period: The period for resampling the data.
    states: include states in the output
    bounds: include bounds in the output
    service_address: service address
    key: API key
    fmt: output format

Raises:
    ValueError: if output format is invalid
"""
client = ReportingApiClient(service_address, key)

metrics = [Metric[mn] for mn in metric_names]

if resampling_period is not None:
    resampling_period = timedelta(seconds=resampling_period)

def data_iter() -> AsyncIterator[MetricSample]:
    """Iterate over single metric.

    Just a wrapper around the client method for readability.

    Returns:
        Iterator over single metric samples
    """
    return client.list_single_component_data(
        microgrid_id=microgrid_id,
        component_id=component_id,
        metrics=metrics,
        start_dt=start_dt,
        end_dt=end_dt,
        resampling_period=resampling_period,
        include_states=states,
        include_bounds=bounds,
    )

But now I am getting (expected) mypy errors?

src/frequenz/client/reporting/main.py:131: error: Incompatible types in assignment (expression has type "timedelta", variable has type "int | None") [assignment]
src/frequenz/client/reporting/main.py:147: error: Argument "resampling_period" to "list_single_component_data" of "ReportingApiClient" has incompatible type "int | None"; expected "timedelta | None" [arg-type]

[Marenz]

Here in the run method, the type should be int | None, not in the ReportingApiClient Constructor if I read it correctly.
You then only need to convert it to timedeltta in line 144.

I would also recommend renaming the parameter to resampling_period_s to indicate that it is seconds in the name right away. Once it is a timedelta that extra _s won't be needed anymore

[cwasicki]

Here in the run method, the type should be int | None

Yes, and below:

resampling_period = timedelta(seconds=resampling_period_s) if resampling_period_s is not None or None
return client.list_single_component_data(
            microgrid_id=microgrid_id,
            component_id=component_id,
            metrics=metrics,
            start_dt=start_dt,
            end_dt=end_dt,
            resampling_period=resampling_period,
            include_states=states,
            include_bounds=bounds,
        )