frequenz-floss / frequenz-api-weather

gRPC+protobuf specification and Python bindings for the Frequenz Weather API
MIT License
1 stars 8 forks source link

Fix to_ndarray_vlf #93

Open david-natingga-frequenz opened 7 months ago

david-natingga-frequenz commented 7 months ago

What happened?

The function to_ndarray_vlf uses location_forecasts[0].forecasts[0].features within the code to get the features. Similarly, the number of times is calculated as num_times = len(location_forecasts[0].forecasts). The problem with this approach is that it assumes that the properties of location_forecasts[0] are true for location_forecasts[i] for all other i too. This may not be always the case, especially for the historical data since for some periods certain features may be missing, while for other periods the features may be there. Another problem is that the function assumes the order of the features is always the same.

What did you expect instead?

The function to_ndarray_vlf should be robust enough to handle cases when instances of location_forecasts are not uniform in their structure, e.g. missing data, different order.

Affected version(s)

No response

Affected part(s)

I don't know (part:❓)

Extra information

Also while the code is being fixed, can it be simplified too? Do we really need the following?

# pylint: disable=too-many-locals,too-many-branches,too-many-statements

Also, please, refactor the function as it is too long.

david-natingga-frequenz commented 7 months ago

Perhaps another point to consider is if we need function to_ndarray_vlf at all given we an alternative function flatten which has a simpler implementation and potentially is also easier to use (?). If flatten is sufficient, we could just remove to_ndarray_vlf.

TalweSingh commented 2 months ago

Might be fixed by #132. Needs to be tested against this case