Repository: ecmwf/earthkit-data (python)

Issue:
Implement valid_datetime metadata key 
### Is your feature request related to a problem? Please describe.

In GRIB data there is no single valid datetime key available, but this piece of information has to be constructed from multiple keys, e.g. from `validityDate` and `validityTime`. At the moment, valid datetime is only available on a GRIB field via the `datetime` method as:

```python
myfield.datetime()["valid_time"]
```

The suggestion is to implement the `valid_datetime` metadata key so that we can access valid datetime as:

```python
myfield.metadata("valid_datetime")
```

This key then could be used in methods like `sel` and `order_by`:


```python
myfieldlist.order_by(valid_datetime="ascending")
```

### Describe the solution you'd like

_No response_

### Describe alternatives you've considered

_No response_

### Additional context

_No response_

### Organisation

ECMWF

Pull request:
Feature/valid datetime metadata key
Fixes #151 

Patch:
diff --git a/earthkit/data/core/fieldlist.py b/earthkit/data/core/fieldlist.py
index f51945a..e1140d3 100644
--- a/earthkit/data/core/fieldlist.py
+++ b/earthkit/data/core/fieldlist.py
@@ -287,7 +287,7 @@ class Field(Base):
             Return types for ``keys``. A single value is accepted and applied to all the ``keys``.
             Otherwise, must have same the number of elements as ``keys``. Only used when
             ``keys`` is not empty.
-        **kwargs: tuple, optional
+        **kwargs: dict, optional
             Other keyword arguments:
 
             * namespace: :obj:`str`, :obj:`list`, :obj:`tuple`, :obj:`None` or :obj:`all`
@@ -395,8 +395,17 @@ class Field(Base):
             if namespace and namespace[0] != "default":
                 key = [namespace[0] + "." + k for k in key]
 
+            raise_on_missing = "default" not in kwargs
+            default = kwargs.pop("default", None)
+
             r = [
-                self._metadata._get(k, astype=kt, **kwargs)
+                self._metadata._get_key(
+                    k,
+                    astype=kt,
+                    default=default,
+                    raise_on_missing=raise_on_missing,
+                    **kwargs,
+                )
                 for k, kt in zip(key, astype)
             ]
 
diff --git a/earthkit/data/core/metadata.py b/earthkit/data/core/metadata.py
index b1809d0..7965bc8 100644
--- a/earthkit/data/core/metadata.py
+++ b/earthkit/data/core/metadata.py
@@ -10,6 +10,8 @@
 
 from abc import ABCMeta, abstractmethod
 
+from earthkit.data.core.constants import DATETIME
+
 
 class Metadata(metaclass=ABCMeta):
     r"""Base class to represent metadata.
@@ -33,7 +35,6 @@ class Metadata(metaclass=ABCMeta):
     def __len__(self):
         r"""Return the number of metadata entries."""
 
-    @abstractmethod
     def __getitem__(self, key):
         r"""Return the value for ``key``.
 
@@ -42,7 +43,7 @@ class Metadata(metaclass=ABCMeta):
         KeyError
             When ``key`` is not available.
         """
-        pass
+        return self._get_key(key)
 
     @abstractmethod
     def __contains__(self, key):
@@ -76,7 +77,6 @@ class Metadata(metaclass=ABCMeta):
         """
         pass
 
-    @abstractmethod
     def get(self, key, default=None):
         r"""Return the value for ``key``.
 
@@ -87,23 +87,60 @@ class Metadata(metaclass=ABCMeta):
         default:
             When ``key`` is not found ``default`` is returned.
         """
-        pass
+        return self._get_key(key, default=default, raise_on_missing=False)
 
-    def _get(self, key, astype=None, **kwargs):
-        # when key is not available and no default is specified this
-        # should raise KeyError
-        if "default" in kwargs:
-            default = kwargs.pop("default")
-            v = self.get(key, default)
-        else:
-            v = self.__getitem__(key)
+    def _get_key(self, key, astype=None, default=None, raise_on_missing=True):
+        r"""Return the value for ``key``.
 
-        if astype is not None:
-            try:
-                return astype(v)
-            except Exception:
-                return None
-        return v
+        Parameters
+        ----------
+        key: str
+            Metadata key
+        default: value
+            Specify the default value for ``key``. Returned when ``key``
+            is not found or its value is a missing value and raise_on_missing is ``False``.
+        raise_on_missing: bool
+            When it is True raises an exception if ``key`` is not found or
+            it has a missing value.
+
+        Returns
+        -------
+        value
+            Returns the ``key`` value. Returns ``default`` if ``key`` is not found
+            or it has a missing value and ``raise_on_missing`` is False.
+
+        Raises
+        ------
+        KeyError
+            If ``raise_on_missing`` is True and ``key`` is not found or it has
+            a missing value.
+
+        """
+        # check for custom keys
+        is_custom, v = self._get_custom_key(
+            key, default=default, raise_on_missing=raise_on_missing
+        )
+        if is_custom:
+            return v
+
+        return self._get_internal_key(
+            key, astype=astype, default=default, raise_on_missing=raise_on_missing
+        )
+
+    @abstractmethod
+    def _get_internal_key(self, key, astype=None, default=None, raise_on_missing=True):
+        pass
+
+    def _get_custom_key(self, key, default=None, raise_on_missing=True):
+        try:
+            if key == DATETIME:
+                return True, self.datetime().get("valid_time")
+        except Exception as e:
+            if not raise_on_missing:
+                return default
+            else:
+                raise KeyError(f"{key}, reason={e}")
+        return False, None
 
     @abstractmethod
     def override(self, *args, **kwargs):
@@ -233,14 +270,21 @@ class RawMetadata(Metadata):
     def items(self):
         return self._d.items()
 
-    def __getitem__(self, key):
-        return self._d[key]
-
     def __contains__(self, key):
         return key in self._d
 
-    def get(self, key, default=None):
-        return self._d.get(key, default)
+    def _get_internal_key(self, key, astype=None, default=None, raise_on_missing=True):
+        if not raise_on_missing:
+            v = self._d.get(key, default)
+        else:
+            v = self._d[key]
+
+        if astype is not None:
+            try:
+                return astype(v)
+            except Exception:
+                return None
+        return v
 
     def override(self, *args, **kwargs):
         d = dict(**self._d)
diff --git a/earthkit/data/readers/grib/codes.py b/earthkit/data/readers/grib/codes.py
index 470d042..eb876fe 100644
--- a/earthkit/data/readers/grib/codes.py
+++ b/earthkit/data/readers/grib/codes.py
@@ -239,13 +239,13 @@ class GribField(Field):
             self._metadata.get("number", None),
         )
 
-    def _get(self, name):
-        """Private, for testing only"""
-        # paramId is renamed as param to get rid of the
-        # additional '.128' (in earthkit/data/scripts/grib.py)
-        if name == "param":
-            name = "paramId"
-        return self.handle.get(name)
+    # def _get(self, name):
+    #     """Private, for testing only"""
+    #     # paramId is renamed as param to get rid of the
+    #     # additional '.128' (in earthkit/data/scripts/grib.py)
+    #     if name == "param":
+    #         name = "paramId"
+    #     return self.handle.get(name)
 
     def write(self, f):
         r"""Writes the message to a file object.
diff --git a/earthkit/data/readers/grib/metadata.py b/earthkit/data/readers/grib/metadata.py
index b0b40b8..1b5520a 100644
--- a/earthkit/data/readers/grib/metadata.py
+++ b/earthkit/data/readers/grib/metadata.py
@@ -223,9 +223,6 @@ class GribMetadata(Metadata):
     def __len__(self):
         return sum(map(lambda i: 1, self._handle.keys()))
 
-    def __getitem__(self, key):
-        return self._handle.get(key)
-
     def __contains__(self, key):
         return self._handle.__contains__(key)
 
@@ -249,19 +246,7 @@ class GribMetadata(Metadata):
         """
         return self._handle.items()
 
-    def get(self, key, default=None):
-        r"""Return the value for ``key``.
-
-        Parameters
-        ----------
-        key: str
-            Metadata key
-        default:
-            When ``key`` is not found ``default`` is returned.
-        """
-        return self._handle.get(key, default=default)
-
-    def _get(self, key, astype=None, **kwargs):
+    def _get_internal_key(self, key, astype=None, default=None, raise_on_missing=False):
         def _key_name(key):
             if key == "param":
                 key = "shortName"
@@ -269,7 +254,11 @@ class GribMetadata(Metadata):
                 key = "paramId"
             return key
 
-        return self._handle.get(_key_name(key), ktype=astype, **kwargs)
+        _kwargs = {}
+        if not raise_on_missing:
+            _kwargs["default"] = default
+
+        return self._handle.get(_key_name(key), ktype=astype, **_kwargs)
 
     def override(self, *args, **kwargs):
         r"""Change the metadata values and return a new object.
@@ -355,6 +344,14 @@ class GribMetadata(Metadata):
     def _base_datetime(self):
         date = self.get("date", None)
         time = self.get("time", None)
+        return self._build_datetime(date, time)
+
+    def _valid_datetime(self):
+        date = self.get("validityDate", None)
+        time = self.get("validityTime", None)
+        return self._build_datetime(date, time)
+
+    def _build_datetime(self, date, time):
         return datetime.datetime(
             date // 10000,
             date % 10000 // 100,
@@ -363,10 +360,6 @@ class GribMetadata(Metadata):
             time % 100,
         )
 
-    def _valid_datetime(self):
-        step = self.get("endStep", None)
-        return self._base_datetime() + datetime.timedelta(hours=step)
-
     def dump(self, namespace=all, **kwargs):
         r"""Generate dump with all the metadata keys belonging to ``namespace``.
 
diff --git a/earthkit/data/sources/list_of_dicts.py b/earthkit/data/sources/list_of_dicts.py
index b0cdb7c..b0b2be2 100644
--- a/earthkit/data/sources/list_of_dicts.py
+++ b/earthkit/data/sources/list_of_dicts.py
@@ -48,7 +48,15 @@ class VirtualGribMetadata(RawMetadata):
     def __init__(self, m):
         super().__init__(m)
 
-    def get(self, key, *args):
+    def _get_internal_key(self, key, astype=None, default=None, raise_on_missing=True):
+        def _key_name(key):
+            if key == "param":
+                key = "shortName"
+            elif key == "_param_id":
+                key = "paramId"
+            return key
+
+        key = _key_name(key)
         key, _, key_type_str = key.partition(":")
         try:
             key_type = self.KEY_TYPES[key_type_str]
@@ -63,36 +71,15 @@ class VirtualGribMetadata(RawMetadata):
                             key = k
                             break
 
-        v = super().get(key, *args)
+        if key_type is None:
+            key_type = astype
 
         if key == "stepRange" and key_type is None:
             key_type = str
 
-        try:
-            if key_type is not None:
-                return key_type(v)
-            else:
-                return v
-        except Exception:
-            return None
-
-    def _get(self, key, astype=None, **kwargs):
-        def _key_name(key):
-            if key == "param":
-                key = "shortName"
-            elif key == "_param_id":
-                key = "paramId"
-            return key
-
-        key = _key_name(key)
-        if astype is not None:
-            key += self.KEY_TYPE_SUFFIX.get(astype)
-
-        if "default" in kwargs:
-            default = kwargs.pop("default")
-            return self.get(key, default)
-        else:
-            return self.get(key)
+        return super()._get_internal_key(
+            key, astype=key_type, default=default, raise_on_missing=raise_on_missing
+        )
 
     def shape(self):
         Nj = self.get("Nj", None)


Test patch:
diff --git a/tests/core/test_metadata.py b/tests/core/test_metadata.py
index 2a4f576..598d881 100644
--- a/tests/core/test_metadata.py
+++ b/tests/core/test_metadata.py
@@ -9,12 +9,14 @@
 # nor does it submit to any jurisdiction.
 #
 
+import datetime
+
 import pytest
 
 from earthkit.data import from_source
 from earthkit.data.core.metadata import RawMetadata
 from earthkit.data.readers.grib.metadata import GribMetadata
-from earthkit.data.testing import earthkit_examples_file
+from earthkit.data.testing import earthkit_examples_file, earthkit_test_data_file
 
 
 @pytest.mark.parametrize(
@@ -157,6 +159,13 @@ def test_grib_metadata_get():
         md.get("centre", "shortName", "step")
 
 
+def test_grib_grib_metadata_valid_datetime():
+    ds = from_source("file", earthkit_test_data_file("t_time_series.grib"))
+    md = ds[4].metadata()
+
+    assert md["valid_datetime"] == datetime.datetime(2020, 12, 21, 18)
+
+
 def test_grib_metadata_override():
     ds = from_source("file", earthkit_examples_file("test.grib"))
     md = ds[0].metadata()
diff --git a/tests/grib/test_grib_metadata.py b/tests/grib/test_grib_metadata.py
index 94eff7b..7167c3e 100644
--- a/tests/grib/test_grib_metadata.py
+++ b/tests/grib/test_grib_metadata.py
@@ -512,6 +512,13 @@ def test_grib_datetime():
     assert s.datetime() == ref
 
 
+def test_grib_valid_datetime():
+    ds = from_source("file", earthkit_test_data_file("t_time_series.grib"))
+    f = ds[4]
+
+    assert f.metadata("valid_datetime") == datetime.datetime(2020, 12, 21, 18)
+
+
 def test_message():
     f = from_source("file", earthkit_examples_file("test.grib"))
     v = f[0].message()
diff --git a/tests/grib/test_grib_order_by.py b/tests/grib/test_grib_order_by.py
index 1b3658f..a5a4747 100644
--- a/tests/grib/test_grib_order_by.py
+++ b/tests/grib/test_grib_order_by.py
@@ -9,6 +9,8 @@
 # nor does it submit to any jurisdiction.
 #
 
+import datetime
+
 import pytest
 
 from earthkit.data import from_source
@@ -157,3 +159,25 @@ def test_grib_order_by_with_sel():
     r = g.order_by({"shortName": "descending"})
     assert len(r) == len(g)
     assert r.metadata("shortName") == ["v", "u", "t"]
+
+
+def test_grib_order_by_valid_datetime():
+    f = from_source("file", earthkit_file("tests/data/t_time_series.grib"))
+
+    g = f.order_by(valid_datetime="descending")
+    assert len(g) == 10
+
+    ref = [
+        datetime.datetime(2020, 12, 23, 12, 0),
+        datetime.datetime(2020, 12, 23, 12, 0),
+        datetime.datetime(2020, 12, 21, 21, 0),
+        datetime.datetime(2020, 12, 21, 21, 0),
+        datetime.datetime(2020, 12, 21, 18, 0),
+        datetime.datetime(2020, 12, 21, 18, 0),
+        datetime.datetime(2020, 12, 21, 15, 0),
+        datetime.datetime(2020, 12, 21, 15, 0),
+        datetime.datetime(2020, 12, 21, 12, 0),
+        datetime.datetime(2020, 12, 21, 12, 0),
+    ]
+
+    assert g.metadata("valid_datetime") == ref
diff --git a/tests/grib/test_grib_sel.py b/tests/grib/test_grib_sel.py
index 8170694..f6c361b 100644
--- a/tests/grib/test_grib_sel.py
+++ b/tests/grib/test_grib_sel.py
@@ -9,6 +9,8 @@
 # nor does it submit to any jurisdiction.
 #
 
+import datetime
+
 import numpy as np
 import pytest
 
@@ -161,6 +163,21 @@ def test_grib_sel_date():
     assert g.metadata(ref_keys) == ref
 
 
+def test_grib_sel_valid_datetime():
+    f = from_source("file", earthkit_file("tests/data/t_time_series.grib"))
+
+    g = f.sel(valid_datetime=datetime.datetime(2020, 12, 21, 21))
+    assert len(g) == 2
+
+    ref_keys = ["shortName", "date", "time", "step"]
+    ref = [
+        ["t", 20201221, 1200, 9],
+        ["z", 20201221, 1200, 9],
+    ]
+
+    assert g.metadata(ref_keys) == ref
+
+
 def test_grib_isel_single_message():
     s = from_source("file", earthkit_file("tests/data/test_single.grib"))
 
diff --git a/tests/netcdf/test_netcdf_metadata.py b/tests/netcdf/test_netcdf_metadata.py
index e5ccbc6..f604c7c 100644
--- a/tests/netcdf/test_netcdf_metadata.py
+++ b/tests/netcdf/test_netcdf_metadata.py
@@ -77,6 +77,11 @@ def test_netcdf_datetime():
     assert ds.datetime() == ref
 
 
+def test_netcdf_valid_datetime():
+    ds = from_source("file", earthkit_examples_file("test.nc"))
+    assert ds[0].metadata("valid_datetime") == datetime.datetime(2020, 5, 13, 12)
+
+
 if __name__ == "__main__":
     from earthkit.data.testing import main