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.

Sign up for GitHub

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

Jump to bottom

Working first version #1

Merged
merged 26 commits into from
Feb 28, 2022
Merged

Working first version #1

merged 26 commits into from
Feb 28, 2022

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Feb 2, 2022 •
edited
Loading

  • CLI command to register a sensor (and create a weather station asset if needed)
  • CLI command to get weather forecasts

NOTE: This requires FlexMeasures/flexmeasures#352 I'll make a dev-version from that branch after the first round of review over there, and then I intend to make the tests work over here. That can be done in a new smaller PR, so this working version is published earlier.

@nhoening nhoening mentioned this pull request Feb 2, 2022
Merged
@nhoening nhoening marked this pull request as ready for review February 11, 2022 13:40
@nhoening nhoening requested a review from Flix6x February 11, 2022 13:40
Flix6x
Flix6x requested changes Feb 14, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flexmeasures_openweathermap/__init__.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/__init__.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/owm.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/owm.py Show resolved Hide resolved
flexmeasures_openweathermap/utils/owm.py Show resolved Hide resolved
flexmeasures_openweathermap/cli/tests/test_get_forecasts.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/cli/tests/test_get_forecasts.py Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor Author

Here is a question: Should we even leave the choice to the user whether they want to setup less than the set of supported sensors per weather station?

In a more simpler world, we let them add a weather station and it gets all supported sensors. Of course, if that means you get six sensors, when all you wanted was one, you have too much data...

@nhoening
Copy link
Contributor Author

But I still fail when a supported sensor has not been registered at all, so I'll work on that.

@nhoening
use server time as TimedBelief.belief_time, also use [FLEXMEASURES-OW…
1c314ce 
…M] as console prefix throughout

Signed-off-by: Nicolas Höning <[email protected]>
@Flix6x
Copy link
Contributor

Flix6x commented Feb 15, 2022

Here is a question: Should we even leave the choice to the user whether they want to setup less than the set of supported sensors per weather station?

In a more simpler world, we let them add a weather station and it gets all supported sensors. Of course, if that means you get six sensors, when all you wanted was one, you have too much data...

Actually, my preference would be to have only the collect_weather_data endpoint, and have it set up a new weather station by itself, in case no station exists at the given location. We could move the sensor_name and sensor_timezone options there, too. But I think that this kind of work is better suited for a follow-up PR.

@nhoening
Copy link
Contributor Author

I believe OWM can't be called for just one sensor at a time. This design is not making sense to me yet

Flix6x
Flix6x requested changes Feb 16, 2022
View reviewed changes
flexmeasures_openweathermap/sensor_specs.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/sensor_specs.py Outdated Show resolved Hide resolved
@nhoening
fit import to FM 0.9
267efc9 
Signed-off-by: Nicolas Höning <[email protected]>
@nhoening
rename sensors, check resolution of found weather sensors, tighter gr…
7860a39 
…ip on test fixtures; de-complexify saving to db function

Signed-off-by: Nicolas Höning <[email protected]>
@nhoening
Copy link
Contributor Author

@Flix6x The compute_irradiance function is still in FM. I propose I also move that over here. Agreed?

@Flix6x
Copy link
Contributor

Flix6x commented Feb 16, 2022

I agree

@nhoening nhoening requested a review from Flix6x February 17, 2022 10:50
Flix6x
Flix6x requested changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a full review yet, but maybe there are already some useful comments in here.

Makefile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flexmeasures_openweathermap/conftest.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/modeling.py Show resolved Hide resolved
flexmeasures_openweathermap/utils/modeling.py Outdated Show resolved Hide resolved
@Flix6x Flix6x mentioned this pull request Feb 23, 2022
Open
Flix6x
Flix6x requested changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little refactoring in the sensor specs requested, which should make more clear how the sensor is set up (while cutting some lines).

flexmeasures_openweathermap/cli/commands.py Show resolved Hide resolved
flexmeasures_openweathermap/sensor_specs.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/sensor_specs.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/cli/commands.py Outdated Show resolved Hide resolved
@nhoening nhoening requested a review from Flix6x February 24, 2022 16:16
Flix6x
Flix6x approved these changes Feb 28, 2022
View reviewed changes
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! FYI, I can verify all tests pass locally on python==3.9.

@nhoening nhoening merged commit debdf07 into main Feb 28, 2022
@nhoening nhoening deleted the working-first-version branch February 28, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flix6x Flix6x Flix6x approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nhoening @Flix6x