Repository: andersbogsnes/ml_tooling

Issue:
API: What to return when no matching data is found when predicting
We should make it clear what return value is expected when .make_prediction fails due to invalid data - either because the key doesn't exist or key is malformed.

Should we raise an MLToolingError? A subclassed InvalidInput or something similar?
We could also return 0 or None, but that feels like failing silently...

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=h1) Report
> Merging [#424](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=desc) into [develop](https://codecov.io/gh/andersbogsnes/ml_tooling/commit/acfa44ef4fd8a0867b4bd03444bc12d9371d8840?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424/graphs/tree.svg?width=650&token=ucKtL3BlxO&height=150&src=pr)](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           develop   #424   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           54     54           
  Lines         1386   1389    +3     
  Branches       157    159    +2     
======================================
+ Hits          1386   1389    +3
```


| [Impacted Files](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/ml\_tooling/data/base\_data.py](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424/diff?src=pr&el=tree#diff-c3JjL21sX3Rvb2xpbmcvZGF0YS9iYXNlX2RhdGEucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [src/ml\_tooling/data/sql.py](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424/diff?src=pr&el=tree#diff-c3JjL21sX3Rvb2xpbmcvZGF0YS9zcWwucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=footer). Last update [acfa44e...affe790](https://codecov.io/gh/andersbogsnes/ml_tooling/pull/424?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b40cc55..27fc088 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 - Fixed typehints in Dataset
 - Dataset.create_train_test now takes a boolean `stratify` parameter.
 - Added default local filestorage when using `save_estimator`
+- Dataset now verifies that `load_training_data` and `load_prediction_data` do not return empty
 
 # v0.10.2
 - Fixed type inferences from data to sql in _load_data
diff --git a/src/ml_tooling/data/base_data.py b/src/ml_tooling/data/base_data.py
index 550823c..88ab4d2 100644
--- a/src/ml_tooling/data/base_data.py
+++ b/src/ml_tooling/data/base_data.py
@@ -104,10 +104,16 @@ class Dataset(metaclass=abc.ABCMeta):
         return self.__class__.__name__
 
     def _load_training_data(self, *args, **kwargs) -> Tuple[pd.DataFrame, DataType]:
-        return self.load_training_data(*args, **kwargs)
+        x, y = self.load_training_data(*args, **kwargs)
+        if x.empty:
+            raise DatasetError("An empty dataset was returned by load_training_data")
+
+        return x, y
 
     def _load_prediction_data(self, *args, **kwargs) -> pd.DataFrame:
         pred_data = self.load_prediction_data(*args, **kwargs)
+        if pred_data.empty:
+            raise DatasetError("An empty dataset was returned by load_prediction_data")
         self.cached_data = pred_data
         return pred_data
 
diff --git a/src/ml_tooling/data/sql.py b/src/ml_tooling/data/sql.py
index 97e12e0..d17aed8 100644
--- a/src/ml_tooling/data/sql.py
+++ b/src/ml_tooling/data/sql.py
@@ -10,7 +10,6 @@ from contextlib import contextmanager
 from ml_tooling.data.base_data import Dataset
 from ml_tooling.utils import DataType, DatasetError
 
-
 logger = logging.getLogger("ml_tooling")
 
 
@@ -102,13 +101,11 @@ class SQLDataset(Dataset, metaclass=abc.ABCMeta):
 
     def _load_training_data(self, *args, **kwargs) -> Tuple[pd.DataFrame, DataType]:
         with self.create_connection() as conn:
-            return self.load_training_data(*args, conn=conn, **kwargs)
+            return super()._load_training_data(*args, conn=conn, **kwargs)
 
     def _load_prediction_data(self, *args, **kwargs) -> pd.DataFrame:
         with self.create_connection() as conn:
-            pred_data = self.load_prediction_data(*args, conn=conn, **kwargs)
-            self.cached_data = pred_data
-            return pred_data
+            return super()._load_prediction_data(*args, conn=conn, **kwargs)
 
     def _dump_data(self) -> pd.DataFrame:
         """


Test patch:
diff --git a/tests/test_baseclass.py b/tests/test_baseclass.py
index eef66b3..3f4879b 100644
--- a/tests/test_baseclass.py
+++ b/tests/test_baseclass.py
@@ -481,7 +481,7 @@ class TestTrainEstimator:
                 self.average = None
 
             def fit(self, x, y=None):
-                self.average = np.mean(x, axis=1)
+                self.average = np.mean(x, axis=0)
                 return self
 
             def predict(self, x):
@@ -489,10 +489,10 @@ class TestTrainEstimator:
 
         class DummyData(Dataset):
             def load_training_data(self):
-                return np.array([[1, 2, 3, 4], [4, 5, 6, 7]]), None
+                return pd.DataFrame({"col1": [1, 2, 3, 4], "col2": [4, 5, 6, 7]}), None
 
             def load_prediction_data(self, *args, **kwargs):
-                return np.array([[1, 2, 3, 4], [4, 5, 6, 7]])
+                return pd.DataFrame({"col1": [1, 2, 3, 4], "col2": [4, 5, 6, 7]})
 
         model = Model(DummyEstimator())
         data = DummyData()
diff --git a/tests/test_data.py b/tests/test_data.py
index 98c8866..62dbe0a 100644
--- a/tests/test_data.py
+++ b/tests/test_data.py
@@ -1,10 +1,12 @@
-import pandas as pd
-import pytest
+import pathlib
 from unittest.mock import MagicMock, patch
-from sqlalchemy.exc import DBAPIError
 
+import pandas as pd
+import pytest
+from ml_tooling import Model
 from ml_tooling.data import Dataset
 from ml_tooling.utils import DatasetError
+from sqlalchemy.exc import DBAPIError
 
 
 class TestDataset:
@@ -39,6 +41,33 @@ class TestDataset:
         dataset.create_train_test(stratify=True)
         assert dataset.has_validation_set is True
 
+    def test_dataset_that_returns_empty_training_data_errors_correctly(self):
+        class FailingDataset(Dataset):
+            def load_training_data(self, *args, **kwargs):
+                return pd.DataFrame(), None
+
+            def load_prediction_data(self, *args, **kwargs):
+                pass
+
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_training_data"
+        ):
+            FailingDataset()._load_training_data()
+
+    def test_dataset_raises_when_load_prediction_data_returns_empty(self, classifier):
+        class FailingDataset(Dataset):
+            def load_training_data(self, *args, **kwargs):
+                pass
+
+            def load_prediction_data(self, *args, **kwargs):
+                return pd.DataFrame()
+
+        data = FailingDataset()
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_prediction_data"
+        ):
+            classifier.make_prediction(data, 0)
+
     def test_cannot_instantiate_an_abstract_baseclass(self):
         with pytest.raises(TypeError):
             Dataset()
@@ -129,6 +158,31 @@ class TestSqlDataset:
             boston_sqldataset(test_engine, "")._dump_data()
         read_sql.assert_called_once()
 
+    def test_sql_dataset_raises_exception_when_load_training_data_returns_empty(
+        self, boston_sqldataset
+    ):
+        class FailingDataset(boston_sqldataset):
+            def load_training_data(self, *args, **kwargs):
+                return pd.DataFrame(), pd.Series()
+
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_training_data"
+        ):
+            FailingDataset("sqlite:///", schema=None).create_train_test()
+
+    def test_sql_dataset_raises_exception_when_load_prediction_data_returns_empty(
+        self, boston_sqldataset, regression
+    ):
+        class FailingDataset(boston_sqldataset):
+            def load_prediction_data(self, *args, **kwargs):
+                return pd.DataFrame()
+
+        data = FailingDataset("sqlite:///", schema=None)
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_prediction_data"
+        ):
+            regression.make_prediction(data, 0)
+
     def test_load_data_throws_error_on_exec_failure(
         self, boston_sqldataset, test_engine
     ):
@@ -204,3 +258,29 @@ class TestFileDataset:
         assert tmp_path.suffix == ""
         with pytest.raises(DatasetError, match="must point to a file"):
             boston_filedataset(tmp_path)
+
+    def test_filedataset_that_returns_empty_training_data_raises_exception(
+        self, boston_csv, boston_filedataset
+    ):
+        class FailingDataset(boston_filedataset):
+            def load_training_data(self, *args, **kwargs):
+                return pd.DataFrame(), pd.Series()
+
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_training_data"
+        ):
+            FailingDataset(boston_csv).create_train_test()
+
+    def test_filedataset_raises_exception_when_load_prediction_data_is_empty(
+        self, regression: Model, boston_filedataset, boston_csv: pathlib.Path
+    ):
+        class FailingDataset(boston_filedataset):
+            def load_prediction_data(self, *args, **kwargs):
+                return pd.DataFrame()
+
+        data = FailingDataset(boston_csv).create_train_test()
+
+        with pytest.raises(
+            DatasetError, match="An empty dataset was returned by load_prediction_data"
+        ):
+            regression.make_prediction(data, 0)