ECMWF: add realtime and restructure #37

Tinkaa · 2021-12-09T08:28:16Z

I added functionality to process the seasonal forecasts with us that are shared in realtime and combine this with the API data.

In that process I also ended up restructuring the API code and renaming files.

It is not fully finished (some todos and questions are marked) but a step in a good direction imo.

…wnload-monica

…DAP/pa-aa-tools into feature/ecmwf-seas-download-monica

Tinkaa · 2021-12-14T11:05:16Z

@turnerm it is now ready for review :) Attempted to implement the class approach but struggling a bit with how to combine the data from the realtime and api the best. So added a few questions and that is why I left it in draft for now (plus tests still have to be added).

turnerm

First of all, I will say that it was a joy to review this PR. Such clear and beautiful code, amazing work Tinka!

There are lots of comments but a good chunk are pretty nit-picky. The three major comments are:

Using pipeline, especially for the realtime class that has Malawi specific paremeters which should go in the country file
The combination class needs some rework, and I'm wondering if it belongs as a class or just a separate method
Unit tests: I just want to get in the habit of including tests with the code rather than waiting until later where they might be forgotten. Not sure how urgent this code is to get up and running though!

src/aatoolbox/utils/geoboundingbox.py

src/aatoolbox/datasources/ecmwf/api_seas5_monthly.py

src/aatoolbox/datasources/ecmwf/realtime_seas5_monthly.py

turnerm · 2021-12-17T10:45:36Z

src/aatoolbox/datasources/ecmwf/combine_seas5_monthly.py

@@ -0,0 +1,103 @@
+"""


I'm wondering: Should this module be in the datasources class, or in our (perhaps to be differently named) aa-toolbox.analysis subpackage (where all postprocessing methods will go).

Is this something that we will likely want to do for many different countries?

How long does it take to run? Does the output file really need to be saved? Could it be saved in exploration instead?

The reason why it is important and needs to be saved is because in pa-anticipatory-action we use it again. But there we also have the CDS source so the user can choose whether to use this data or the cds data. If choosing to use directly the ecmwf data it makes sense imo that you get one dataset that contains the combined api and realtime data. Would you agree?

It doesn't take super long to run so maybe somehow we don't have to save it if we port everything to aa-toolbox but not sure if that will work

Would probably run it for each country that uses the ecmwf data

get one dataset that contains the combined api and realtime data

I guess my hesitation is that it doesn't really fit into the paradigm of what datasource is supposed to be, i.e. the download, cleaning, and loading of data from a single source. The combination of sources feels more like post-processing to me. Do you think the results could be saved in exploration?

Uff these edge cases are always the hardest 😅 I will think about it a bit more.

turnerm · 2021-12-17T10:46:58Z

src/aatoolbox/datasources/ecmwf/combine_seas5_monthly.py

+        Path to processed NetCDF file
+
+        """
+        ecmwf_rt = EcmwfRealtime(iso3=self._iso3, geobb=self._geobb)


I would not call the classes here, but rather pass their outputs as input paremeters. This can be handled by the pipeline

turnerm · 2021-12-17T10:48:34Z

src/aatoolbox/datasources/ecmwf/combine_seas5_monthly.py

+            self._geobb = GeoBoundingBox(north=90, south=-90, east=0, west=360)
+        # round coordinates to correspond with the grid ecmwf publishes
+        # its data on
+        self._geobb.round_coords(round_val=GRID_RESOLUTION)


I would also not set this but rather pass it in from the other ECMWF class

turnerm · 2021-12-17T10:50:40Z

src/aatoolbox/datasources/ecmwf/combine_seas5_monthly.py

+GRID_RESOLUTION = 0.4  # degrees
+
+
+# question: should Ecmwf be a super class of EcmwfApi and EcmwfRealtime?


No because most of the methods would just override each other from the two classes since they all have the same names (almost). Multiple inheritance is more useful (I think) if you have two very different classes.

As mentioned above, I'm not sure this makese sense as a class, but I'd be interested to see how it looks once you've made the changes I suggested below.

Tinkaa · 2021-12-24T09:57:08Z

src/aatoolbox/utils/geoboundingbox.py

@@ -89,6 +89,7 @@ def round_boundingbox_coords(
            elif direction in ("south", "west"):
                function = np.floor.__call__  # needed for mypy
                offset_factor = -1
+


arghh the rounded_coord has to deal with floating inprecision. So we get outputs like 32.80000000000000182076576039 while we would like just 32.8. This is problematic cause when round_coords() is then called twice, it rounds up again which we wouldn't want.

@turnerm Do you know a solution for this? The internet seems to suggest using Decimal() but wondering if you have a better idea?

Ah yes I think using decimal would make a lot of sense here. Furthermore, it might be good to make it only possible to run round_coords once (how is it getting run twice?). But this could be moved to an issue.

👍 it is being run twice due to the lovely combination class but even then I think it would be good to have a check that it isn't accidentally ran twice in some way. Do you know how we could do that?

You could make an instance attributed called _is_rounded that is set to True once the coords have been rounded, and then when running round_coords just check this attribute and exit out if it's True

src/aatoolbox/datasources/ecmwf/realtime_seas5_monthly.py

kynan · 2022-11-13T21:03:04Z

src/aatoolbox/datasources/ecmwf/api_seas5_monthly.py

+
+    def download(
+        self,
+        min_date: Union[str, date] = None,


Suggested change

min_date: Union[str, date] = None,

min_date: Union[str, date, None] = None,

kynan · 2022-11-13T21:03:16Z

src/aatoolbox/datasources/ecmwf/api_seas5_monthly.py

+    def download(
+        self,
+        min_date: Union[str, date] = None,
+        max_date: Union[str, date] = None,


Suggested change

max_date: Union[str, date] = None,

max_date: Union[str, date, None] = None,

kynan · 2022-11-13T21:03:45Z

src/aatoolbox/datasources/ecmwf/api_seas5_monthly.py

+        """
+        return xr.load_dataset(self._get_processed_path())
+
+    def _get_raw_path(self, date_forecast: Union[pd.Timestamp, None]):


Suggested change

def _get_raw_path(self, date_forecast: Union[pd.Timestamp, None]):

def _get_raw_path(self, date_forecast: Optional[pd.Timestamp]):

kynan · 2022-11-13T21:04:03Z

src/aatoolbox/datasources/ecmwf/api_seas5_monthly.py

+import logging
+from datetime import date
+from pathlib import Path
+from typing import Union


Suggested change

from typing import Union

from typing import Optional, Union

kynan · 2022-11-13T21:09:35Z

src/aatoolbox/datasources/ecmwf/combine_seas5_monthly.py

+        )
+        # the geo_bounding_box indicates the boundaries for which data is
+        # downloaded and processed
+        if type(geo_bounding_box) == gpd.GeoDataFrame:


Suggested change

if type(geo_bounding_box) == gpd.GeoDataFrame:

if isinstance(geo_bounding_box, gpd.GeoDataFrame):

kynan · 2022-11-14T17:45:42Z

src/aatoolbox/pipeline.py

@@ -101,3 +105,101 @@ def _load_codab(self, layer_name: str, clobber: bool):
            clobber=clobber,
        )
        return self._codab.load_admin_layer(layer_name=layer_name)
+
+    class Coordinates(TypedDict):


Could also use a NamedTuple here

kynan · 2022-11-14T17:46:08Z

src/aatoolbox/pipeline.py

@@ -1,8 +1,12 @@
 """Pipeline initializer."""
+from typing import TypedDict


Suggested change

from typing import TypedDict

from typing import Optional, TypedDict

kynan · 2022-11-14T17:46:18Z

src/aatoolbox/pipeline.py

+        self,
+        from_codab: bool = True,
+        from_config: bool = False,
+        coordinates: Coordinates = None,


Suggested change

coordinates: Coordinates = None,

coordinates: Optional[Coordinates] = None,

kynan · 2022-11-14T17:46:59Z

src/aatoolbox/pipeline.py

+        from_codab: bool = True,
+        from_config: bool = False,
+        coordinates: Coordinates = None,
+    ):


Suggested change

):

) -> GeoBoundingBox:

kynan · 2022-11-14T17:50:10Z

src/aatoolbox/pipeline.py

+                "None of the options given to retrieve a geoboundingbox."
+            )
+
+    def load_ecmwf_realtime(self, process: bool = False):


Suggested change

def load_ecmwf_realtime(self, process: bool = False):

def load_ecmwf_realtime(self, process: bool = False) -> xr.Dataset:

Tinkaa and others added 19 commits November 1, 2021 16:18

ecmwf api example download

08f3dc9

Merge branch 'develop' into feature/ecmwf-seas-download-monica

0a228b1

Updated reqs

c44b5dd

Added ecmwf api processing

c5dd29b

Merge branch 'develop' into feature/ecmwf-seas-download

e713199

add lost req file

8b7ec5e

Merge branch 'feature/ecmwf-seas-download' into feature/ecmwf-seas-do…

f570564

…wnload-monica

Remoed datetime

a01297f

Fixed leadtime and time parameter

b3bad16

Added mkdir for processed output

b9bd5bc

Remove month subtraction

530afa1

Add grid parameter

0f2f670

attempt to add file existence

fb673db

Merge branch 'feature/ecmwf-seas-download-monica' of github.com:OCHA-…

f84763e

…DAP/pa-aa-tools into feature/ecmwf-seas-download-monica

function to download for list of dates

25f535a

change datelist

74dd412

Added rounding

9ef408c

Removed debug lines

da731ce

add realtime and restructure

8d4b53a

Tinkaa requested a review from turnerm December 9, 2021 08:28

Tinkaa added 2 commits December 9, 2021 13:31

improve preprocessing netcdf

9c510f6

create ecmwfapi class

c7d267f

Tinkaa marked this pull request as draft December 9, 2021 15:48

Tinkaa changed the base branch from feature/ecmwf-seas-download-monica to develop December 9, 2021 16:19

This was referenced Dec 9, 2021

Feature/ecmwf seas download #21

Closed

ECMWF API seasonal processing #36

Closed

Tinkaa added 4 commits December 9, 2021 17:23

refactor area to utils

e435098

Merge branch 'feature/area-refactor' into feature/ecmwf-seas-realtime

e9702b9

add realtime class and combine

00752c5

Merge branch 'develop' into feature/ecmwf-seas-realtime

7a7d678

fix area and add load

5fa0170

Tinkaa mentioned this pull request Dec 14, 2021

ecmwf realtime data: retrieve from AWS bucket #41

Open

Tinkaa added 2 commits December 14, 2021 11:53

Merge branch 'develop' into feature/ecmwf-seas-realtime

10ee2d0

replace area by geoboundingbox

9dda407

add step dim for merging

f7159cb

Tinkaa mentioned this pull request Dec 16, 2021

test realtime data OCHA-DAP/pa-anticipatory-action#248

Closed

turnerm reviewed Dec 17, 2021

View reviewed changes

Tinkaa added 4 commits December 23, 2021 16:21

first set of great comments

badf26b

second batch comments

a35e069

pr comments batch 3

59d031a

pr comments batch 3

a01d06d

Tinkaa commented Dec 24, 2021

View reviewed changes

remove src in imports

a827d4c

Tinkaa commented Jan 6, 2022

View reviewed changes

src/aatoolbox/datasources/ecmwf/realtime_seas5_monthly.py Outdated Show resolved Hide resolved

Tinkaa added 3 commits January 6, 2022 12:16

combine dims multiple dates

0babf59

move vars to config

b5e43a6

move geobb to pipeline

0a203f6

Tinkaa mentioned this pull request Mar 1, 2022

Add ECMWF seasonal #70

Open

Tinkaa mentioned this pull request Mar 29, 2022

ecmwf: add start time coord #43

Closed

Tinkaa added 3 commits September 16, 2022 15:07

add init to ecmwf

13abfbc

remove unused match_realtime

ff37069

change dir

104953a

kynan reviewed Nov 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECMWF: add realtime and restructure #37

ECMWF: add realtime and restructure #37

Tinkaa commented Dec 9, 2021

Tinkaa commented Dec 14, 2021

turnerm left a comment

turnerm Dec 17, 2021

Tinkaa Dec 24, 2021

turnerm Dec 24, 2021

turnerm Dec 17, 2021

turnerm Dec 17, 2021

turnerm Dec 17, 2021

Tinkaa Dec 24, 2021

turnerm Dec 24, 2021

Tinkaa Dec 24, 2021

turnerm Dec 24, 2021

kynan Nov 13, 2022

kynan Nov 13, 2022

kynan Nov 13, 2022

kynan Nov 13, 2022

kynan Nov 13, 2022

kynan Nov 14, 2022

kynan Nov 14, 2022

kynan Nov 14, 2022

kynan Nov 14, 2022

kynan Nov 14, 2022

		GRID_RESOLUTION = 0.4 # degrees


		# question: should Ecmwf be a super class of EcmwfApi and EcmwfRealtime?

	min_date: Union[str, date] = None,
	min_date: Union[str, date, None] = None,

	max_date: Union[str, date] = None,
	max_date: Union[str, date, None] = None,

	def _get_raw_path(self, date_forecast: Union[pd.Timestamp, None]):
	def _get_raw_path(self, date_forecast: Optional[pd.Timestamp]):

	if type(geo_bounding_box) == gpd.GeoDataFrame:
	if isinstance(geo_bounding_box, gpd.GeoDataFrame):

		@@ -1,8 +1,12 @@
		"""Pipeline initializer."""
		from typing import TypedDict

	from typing import TypedDict
	from typing import Optional, TypedDict

	coordinates: Coordinates = None,
	coordinates: Optional[Coordinates] = None,

	def load_ecmwf_realtime(self, process: bool = False):
	def load_ecmwf_realtime(self, process: bool = False) -> xr.Dataset:

ECMWF: add realtime and restructure #37

Are you sure you want to change the base?

ECMWF: add realtime and restructure #37

Conversation

Tinkaa commented Dec 9, 2021

Tinkaa commented Dec 14, 2021

turnerm 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

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