frequenz-floss / frequenz-api-weather

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

Convert a page of historical location forecast data into a flattened numpy array for easier use #89

Closed david-natingga-frequenz closed 5 months ago

david-natingga-frequenz commented 6 months ago

[342 rows x 6 columns]



Closes https://github.com/frequenz-floss/frequenz-api-weather/issues/88.
TalweSingh commented 6 months ago

Also, split the code changes to the client and the example in 2 commits. And a more descriptive title for the PR itself would be great

david-natingga-frequenz commented 6 months ago

Also, split the code changes to the client and the example in 2 commits. And a more descriptive title for the PR itself would be great

I made the title more descriptive.

As for putting an example into a separate commit, it is problematic because the old example would not work with the new code. The reason is that the historical data iterator is changed in this PR, it returns a different object.

shsms commented 6 months ago

Looks like the lack of tests is starting to be a problem. I hope you've tested there's no changes in behaviour for live streaming from the changes to shared methods.

david-natingga-frequenz commented 6 months ago

Looks like the lack of tests is starting to be a problem. I hope you've tested there's no changes in behaviour for live streaming from the changes to shared methods.

I think the method to_ndarray_vlf is quite complicated and I do not see in it much use. In https://github.com/frequenz-floss/frequenz-api-weather/issues/93 I propose just to remove it. @shsms Do you know any code depending on that method? In this PR, I just move the method out, but I do not modify it, so its behaviour should not change.

As for my testing, I just run the live streaming locally and it works with this code just as it worked with the code before. However, you are right, there are no tests that would warrant the exact behaviour as before.

cwasicki commented 6 months ago

@david-natingga-frequenz This is very difficult to review. One reason is, that there are too many changes in a single commit. Can this be split into separate commits?

Do you know any code depending on that method?

Yes, our forecasts.

However, you are right, there are no tests that would warrant the exact behaviour as before.

We should have these.

david-natingga-frequenz commented 6 months ago

Based on the feedback of all of you, I simplified this PR and put it into 2 commits.

@cwasicki Please, have a look.

david-natingga-frequenz commented 5 months ago

@cwasicki Ready for a review again.