Skip to content
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

Fix to_ndarray_vlf #93

Open
david-natingga-frequenz opened this issue Apr 4, 2024 · 2 comments
Open

Fix to_ndarray_vlf #93

david-natingga-frequenz opened this issue Apr 4, 2024 · 2 comments
Assignees
Labels
part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:bug Something isn't working
Milestone

Comments

@david-natingga-frequenz
Copy link
Contributor

david-natingga-frequenz commented Apr 4, 2024

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 david-natingga-frequenz added priority:❓ We need to figure out how soon this should be addressed type:bug Something isn't working labels Apr 4, 2024
@keywordlabeler keywordlabeler bot added the part:❓ We need to figure out which part is affected label Apr 4, 2024
@david-natingga-frequenz
Copy link
Contributor Author

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.

@cwasicki cwasicki added this to the v1.0.0 milestone Sep 10, 2024
@TalweSingh
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants