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

Get forecasts for an asset-id instead of passing location #16

Conversation

Ahmad-Wahid
Copy link
Contributor

Changes:

  • User can now pass --asset-id to get forecasts for all the registered sensor(s) of the given asset. This is an alternative of --location.
  • Now using GenericAsset(Type) instead of Asset(Type).

flexmeasures_openweathermap/cli/commands.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/cli/commands.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/locating.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/locating.py Show resolved Hide resolved
flexmeasures_openweathermap/utils/locating.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/cli/commands.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nhoening nhoening 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 better suggestion for the exception message.

flexmeasures_openweathermap/utils/locating.py Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor

I believe we should also add the --asset-id parameter to the owm register-weather-sensor CLI command.

Then both CLI commands let you specify the exact weather station (by ID) that should be used.

…ame-to-cli-commands-instead-of-full-location' into 2-allow-to-pass-asset-id-andor-name-to-cli-commands-instead-of-full-location

# Conflicts:
#	flexmeasures_openweathermap/cli/commands.py
#	flexmeasures_openweathermap/utils/locating.py
…-id-andor-name-to-cli-commands-instead-of-full-location
@Ahmad-Wahid
Copy link
Contributor Author

Ahmad-Wahid commented Jul 26, 2023

Is this asset-id required for sensor registration?

By both commands, you mean asset-id and (lat and long that are required for sensor registration)?

@nhoening
Copy link
Contributor

Is this asset-id required for sensor registration?

To identify the weather station bi ID, not by location.

By both commands, you mean asset-id and (lat and long that are required for sensor registration)?

I mean the two CLI commands we have in this plugin, sensor registration and forecast retrieval. We worked already on the latter.

@Ahmad-Wahid
Copy link
Contributor Author

Okay, understood. Now if I pass asset-id to registor a weather sensor, the weather station should be created before this step and it requires latitude and longitude.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small suggestions.

flexmeasures_openweathermap/utils/locating.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/modeling.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/cli/schemas/weather_sensor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks

flexmeasures_openweathermap/cli/commands.py Show resolved Hide resolved
flexmeasures_openweathermap/cli/schemas/weather_sensor.py Outdated Show resolved Hide resolved
flexmeasures_openweathermap/utils/locating.py Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor

@Ahmad-Wahid we still have a failing test (probably because we now are not raising where we used to before:

def test_get_weather_forecasts_no_close_sensors(
        app, db, monkeypatch, run_as_cli, add_weather_sensors_fresh_db
    ):
        """
        Looking for a location too far away from existing weather stations means we fail.
        """
        weather_station = add_weather_sensors_fresh_db["wind"].generic_asset
    
        monkeypatch.setitem(app.config, "OPENWEATHERMAP_API_KEY", "dummy")
        monkeypatch.setattr(owm, "call_openweatherapi", mock_owm_response)
    
        runner = app.test_cli_runner()
        result = runner.invoke(
            collect_weather_data,
            ["--location", f"{weather_station.latitude-5},{weather_station.longitude}"],
        )
        print(result.output)
>       assert "Reported task get-openweathermap-forecasts status as False" in result.output
E       AssertionError: assert 'Reported task get-openweathermap-forecasts status as False' in '[FLEXMEASURES-OWM] Only one location: 28.4843866,126.0.\n[FLEXMEASURES-OWM] Getting weather forecasts:\n[FLEXMEASURES...ask get-openweathermap-forecasts ran fine.\n[FLEXMEASURES] Reported task get-openweathermap-forecasts status as True\n'
E        +  where '[FLEXMEASURES-OWM] Only one location: 28.4843866,126.0.\n[FLEXMEASURES-OWM] Getting weather forecasts:\n[FLEXMEASURES...ask get-openweathermap-forecasts ran fine.\n[FLEXMEASURES] Reported task get-openweathermap-forecasts status as True\n' = <Result okay>.output

Please adjust the test, then this PR can be merged.

@nhoening nhoening merged commit 42bed45 into main Mar 12, 2024
2 checks passed
@nhoening nhoening deleted the 2-allow-to-pass-asset-id-andor-name-to-cli-commands-instead-of-full-location branch March 12, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to pass asset ID and/or name to CLI commands instead of full location
2 participants