Enabling multiple weatherforecastlocations selection #146

maxi-fort · 2024-04-26T15:30:51Z

The evolution enables multiple weatherforecastlocations selection.

Default is one weatherforecastlocation (the nearest) so it does not impact the original behaviour of the code.

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Signed-off-by: black <action@github.com>

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Signed-off-by: black <action@github.com>

clara-de-smet · 2024-05-16T13:33:32Z

openstef_dbc/services/weather.py

@@ -55,6 +55,7 @@ def _get_nearest_weather_location(
        location: Union[Tuple[float, float], str],
        country: str = "NL",
        threshold: float = 150.0,
+        k: int = 1,


Could you make this parameter name more descriptive? E.g. number_locations?

You're right, il will change it to number_locations

Hi @clara-de-smet,
the parameter k has been renamed to number_locations :

openstef-dbc/openstef_dbc/services/weather.py

Lines 53 to 59 in 009e60f

def _get_nearest_weather_locations(

self,

location: Union[Tuple[float, float], str],

country: str = "NL",

threshold: float = 150.0,

number_locations: int = 1,

) -> pd.Series:

JanMaartenvanDoorn

Thanks for adding this feature! See the comments for things that I found. In general it is probably worth to change the output from _get_nearest_weather_locations to a list of strings instead of supporting both a single string and a list of strings. Contact me via teams or email if you have any questions or want to discuss a little.

JanMaartenvanDoorn · 2024-05-16T13:48:11Z

openstef_dbc/services/weather.py


        # Find closest weather location
        if distances.index.min() < threshold:
            if isinstance(nearest_location, pd.Series):
-                return nearest_location.reset_index(drop=True)[0]
+                return nearest_location.reset_index(drop=True)


The output of this function should be string (name of the weather location).

I see in line 108 nearest weather location is created as a list, the if statement in line 112 checks if nearest location is a pd.Series which will never be the case. Probably you do not want to convert the pd.Series to a list in line 108.

If you want to return mulitple weather locations,I think it is good to update the output type hints and documentation that a list of strings is returned instead of just a string. Maybe also change the name "nearest_location" into "nearest_locations"

Indeed.
I will apply modifications so that the output is a list of str

Hi @JanMaartenvanDoorn,

I changed the class of the object returned :
It is now a dataframe with one column "input_city" containing the name of the nearest(s) weather location(s), and the distance (in km) as index.

Such information my be of interest for someone who desire to compute spatial weighted averaging of weather indices (which is RTE's case).

Therefore, the 'list' of input_city is extracted from this dataframe just before db query :

openstef-dbc/openstef_dbc/services/weather.py

Lines 303 to 308 in 009e60f

# Initialise strings for the querying influx, it is not possible to parameterize this string

weather_params_str = '" or r._field == "'.join(weatherparams)

weather_models_str = '" or r.source == "'.join(source)

weather_location_name_str = '" or r.input_city == "'.join(

location_name.to_list()

)

JanMaartenvanDoorn · 2024-05-16T13:52:49Z

openstef_dbc/services/weather.py

@@ -215,6 +217,7 @@ def get_weather_data(
        source: Union[List[str], str] = "optimum",
        resolution: str = "15min",
        country: str = "NL",
+        k: int = 1,


Same as @clara-de-smet mentioned this should be more descriptive e.g. max_number_of_weather_locations

I will use @clara-de-smet suggestion

@JanMaartenvanDoorn , k has been renamed to number_locations

JanMaartenvanDoorn · 2024-05-16T13:53:57Z

openstef_dbc/services/weather.py


        # Make a list of the source and weatherparams.
        # Like this, it also works if source is a string instead of multiple values
        if isinstance(source, str):
            source = [source]
        if isinstance(weatherparams, str):
            weatherparams = [weatherparams]
+        if isinstance(location_name, str):


If we decide we only return a list of strings in _get_nearest_weather_location, we do not have to do this conversion and check.

removing this

JanMaartenvanDoorn · 2024-05-16T13:57:43Z

tests/unit/services/test_radiation_shift.py

Nice to add unit tests!

I also added a 2 unit tests in test_weather.py :

openstef-dbc/tests/unit/services/test_weather.py

Line 117 in 009e60f

def test_get_multiple_location_weather_data(self, MockDataInterface):

openstef-dbc/tests/unit/services/test_weather.py

Line 187 in 009e60f

def test_get_single_location_weather_data(self, MockDataInterface):

JanMaartenvanDoorn · 2024-05-16T13:58:15Z

openstef_dbc/services/weather.py

-        result = result.loc[datetime_start_original:]
+        result = result[result.index >= datetime_start_original]
+
+        if k == 1:


Here is another k which is not so descriptive

This have been changed.
Plus, I added a merge with location_name dataframe to add "distiance" column to the returned dataframe if number_locations > 1

openstef-dbc/openstef_dbc/services/weather.py

Lines 362 to 372 in 009e60f

if number_locations == 1:

result = result.drop(columns="input_city")

else:

# Adding distance information en km

result = (

result.reset_index()

.merge(location_name.reset_index(), how="left", on="input_city")

.set_index("datetime")

)

return result

openstef_dbc/services/weather.py

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Signed-off-by: black <action@github.com>

JanMaartenvanDoorn

Looks good! Thanks for implementing these changes!

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

…of-only-1' of https://github.com/maxi-fort/openstef-dbc into 145-extract-multiple-nearest-weather-locations-instead-of-only-1

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Signed-off-by: black <action@github.com>

maxi-fort · 2024-07-05T12:22:05Z

@JanMaartenvanDoorn and @clara-de-smet ,
Your comment has been taken into account but I modified a few things since, like :

keeping distance in get_nearest_weather_locations
adding additionnal columns in get_weather_data output
Could you review this PR once again and mark conversations as "resolved" when it is ok ?
Thanks in advance

maxi-fort linked an issue Apr 26, 2024 that may be closed by this pull request

Extract multiple nearest weather locationS instead of only 1 #145

Open

Enabling multiple weatherforecastlocations selection

b91a37e

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

maxi-fort force-pushed the 145-extract-multiple-nearest-weather-locations-instead-of-only-1 branch from e9a2c72 to b91a37e Compare April 26, 2024 15:37

maxi-fort and others added 4 commits April 30, 2024 11:03

adding get_weather_data compatibility

3f2ecec

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Format Python code with Black

271f5ba

Signed-off-by: black <action@github.com>

Updating test with regard to get_nearst_weather_location new argument

d3b031c

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Format Python code with Black

c41f899

Signed-off-by: black <action@github.com>

maxi-fort requested a review from JanMaartenvanDoorn May 16, 2024 13:32

maxi-fort assigned JanMaartenvanDoorn May 16, 2024

clara-de-smet reviewed May 16, 2024

View reviewed changes

JanMaartenvanDoorn requested changes May 16, 2024

View reviewed changes

maxi-fort and others added 4 commits May 24, 2024 11:02

Changing output type and argument and method's name

0c76c43

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Adding unit tests

4379c36

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Adapting other tests to get_nearest_weather_locations new output type

893bfe1

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Format Python code with Black

44d4a10

Signed-off-by: black <action@github.com>

maxi-fort requested review from clara-de-smet and JanMaartenvanDoorn May 24, 2024 09:23

JanMaartenvanDoorn approved these changes May 27, 2024

View reviewed changes

maxi-fort and others added 4 commits July 2, 2024 18:59

adding distance column when multiple locations are requested

ab66870

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Merge branch '145-extract-multiple-nearest-weather-locations-instead-…

3b2d263

…of-only-1' of https://github.com/maxi-fort/openstef-dbc into 145-extract-multiple-nearest-weather-locations-instead-of-only-1

black formatting

b4e7046

Signed-off-by: fortinmax <maxime.fortin@rte-france.com>

Format Python code with Black

009e60f

Signed-off-by: black <action@github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling multiple weatherforecastlocations selection #146

Enabling multiple weatherforecastlocations selection #146

maxi-fort commented Apr 26, 2024

clara-de-smet May 16, 2024

maxi-fort May 23, 2024

maxi-fort Jul 5, 2024

JanMaartenvanDoorn left a comment

JanMaartenvanDoorn May 16, 2024

maxi-fort May 23, 2024

maxi-fort Jul 5, 2024

JanMaartenvanDoorn May 16, 2024

maxi-fort May 23, 2024

maxi-fort Jul 5, 2024

JanMaartenvanDoorn May 16, 2024

maxi-fort May 23, 2024

JanMaartenvanDoorn May 16, 2024

maxi-fort Jul 5, 2024

JanMaartenvanDoorn May 16, 2024

maxi-fort Jul 5, 2024

JanMaartenvanDoorn left a comment

maxi-fort commented Jul 5, 2024 •

edited

Loading

	def _get_nearest_weather_locations(
	self,
	location: Union[Tuple[float, float], str],
	country: str = "NL",
	threshold: float = 150.0,
	number_locations: int = 1,
	) -> pd.Series:

	# Initialise strings for the querying influx, it is not possible to parameterize this string
	weather_params_str = '" or r._field == "'.join(weatherparams)
	weather_models_str = '" or r.source == "'.join(source)
	weather_location_name_str = '" or r.input_city == "'.join(
	location_name.to_list()
	)

	if number_locations == 1:
	result = result.drop(columns="input_city")
	else:
	# Adding distance information en km
	result = (
	result.reset_index()
	.merge(location_name.reset_index(), how="left", on="input_city")
	.set_index("datetime")
	)

	return result

Enabling multiple weatherforecastlocations selection #146

Are you sure you want to change the base?

Enabling multiple weatherforecastlocations selection #146

Conversation

maxi-fort commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanMaartenvanDoorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanMaartenvanDoorn left a comment

Choose a reason for hiding this comment

maxi-fort commented Jul 5, 2024 • edited Loading

maxi-fort commented Jul 5, 2024 •

edited

Loading