Repository: EducationalTestingService/rsmtool
Pull request #641 (2023-08-08 18:02:28)

Improvements to fast_predict validation/error handling
Deal with situation where trimming, scaling, etc., modeler attributes exist but are `None`, which would be the case when loading an older model (bare SKLL model).

Even if these attributes were `None` before, it would still have led to an error somewhere down the line (`TypeError` if `trim_min` and `trim_tolerance` were `None` and/or a mixture of `None` and `float`, for instance). This change should catch that and give a nicer error message.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService) Report
Patch coverage: **`100.00%`** and project coverage change: **`+0.01%`** :tada:
> Comparison is base [(`f22a6f5`)](https://app.codecov.io/gh/EducationalTestingService/rsmtool/commit/f22a6f5da418751dda9f2dd4dc8185410f84ddcc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService) 95.80% compared to head [(`6459fc6`)](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService) 95.81%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   95.80%   95.81%   +0.01%     
==========================================
  Files          32       32              
  Lines        4432     4444      +12     
==========================================
+ Hits         4246     4258      +12     
  Misses        186      186              
```


| [Files Changed](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService) | Coverage Δ | |
|---|---|---|
| [rsmtool/modeler.py](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService#diff-cnNtdG9vbC9tb2RlbGVyLnB5) | `97.65% <100.00%> (+<0.01%)` | :arrow_up: |
| [rsmtool/rsmpredict.py](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService#diff-cnNtdG9vbC9yc21wcmVkaWN0LnB5) | `99.39% <100.00%> (+0.04%)` | :arrow_up: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/EducationalTestingService/rsmtool/pull/641?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EducationalTestingService).

mulhod: @blongwill Please take a look.

Patch:
diff --git a/rsmtool/modeler.py b/rsmtool/modeler.py
index ce0cb45d..160376a5 100644
--- a/rsmtool/modeler.py
+++ b/rsmtool/modeler.py
@@ -41,6 +41,7 @@ class Modeler:
         The learner and the scaling-/trimming-related attributes are
         set to ``None``.
         """
+        self.feature_info = None
         self.learner = None
         self.trim_min = None
         self.trim_max = None
diff --git a/rsmtool/rsmpredict.py b/rsmtool/rsmpredict.py
index cb8af222..c81f1f08 100644
--- a/rsmtool/rsmpredict.py
+++ b/rsmtool/rsmpredict.py
@@ -173,12 +173,17 @@ def fast_predict(
     Raises
     ------
     ValueError
-        If ``input_features`` contains any non-numeric features; if
-        trimming/scaling is turned on but related parameters are either
-        not specified or cannot be found as attributes in ``modeler``;
-        if trimming/scaling-related parameters are specified but
-        trimming/scaling is turned off; if feature information is either
-        not specified or cannot be found as an attribute in ``modeler``.
+        If ``input_features`` contains any non-numeric features
+    ValueError
+        If trimming/scaling is turned on but related parameters are either
+        not specified or cannot be found as attributes in ``modeler``/have
+        a value of ``None``
+    ValueError
+        If trimming/scaling-related parameters are specified but
+        trimming/scaling is turned off
+    ValueError
+        If feature information is either not specified or cannot be found
+        as an attribute in ``modeler``/has a value of ``None``
     """
     # initialize a logger if none provided
     logger = logger if logger else logging.getLogger(__name__)
@@ -190,14 +195,18 @@ def fast_predict(
     df_input_features = pd.DataFrame([input_features])
     df_input_features["spkitemid"] = "RESPONSE"
 
+    feature_info_error_message = (
+        "'df_feature_info' must be specified if it not found as an attribute in the "
+        "modeler object with a value that is not ``None``"
+    )
     if df_feature_info is None:
         try:
-            df_feature_info = modeler.feature_info
-        except AttributeError:
-            raise ValueError(
-                "df_feature_info must be specified if it cannot be found as an "
-                "attribute in the modeler object"
-            ) from None
+            stored_feature_info = modeler.feature_info
+            assert stored_feature_info is not None
+        except (AttributeError, AssertionError):
+            raise ValueError(feature_info_error_message) from None
+        else:
+            df_feature_info = stored_feature_info
 
     # preprocess the input features so that they match what the model expects
     try:
@@ -212,6 +221,11 @@ def fast_predict(
 
     # compute scaled predictions if requested
     if scale:
+        scale_args_error_message = (
+            "When 'scale' is set to True and no explicit values are provided, the "
+            "'train_predictions_mean', 'train_predictions_sd', 'h1_mean', and 'h1_sd' "
+            "modeler attributes must be present and not ``None``."
+        )
         try:
             if train_predictions_mean is None:
                 train_predictions_mean = modeler.train_predictions_mean
@@ -221,12 +235,13 @@ def fast_predict(
                 h1_mean = modeler.h1_mean
             if h1_sd is None:
                 h1_sd = modeler.h1_sd
+            if any(
+                arg is None
+                for arg in [train_predictions_mean, train_predictions_sd, h1_mean, h1_sd]
+            ):
+                raise ValueError(scale_args_error_message) from None
         except AttributeError:
-            raise ValueError(
-                "The train_predictions_mean/train_predictions_sd/h1_mean/h1_sd modeler"
-                " attributes must exist when scale=True and these values are not "
-                "explicitly specified"
-            ) from None
+            raise ValueError(scale_args_error_message) from None
         df_predictions["scale"] = (
             (df_predictions["raw"] - train_predictions_mean) / train_predictions_sd
         ) * h1_sd + h1_mean
@@ -243,10 +258,17 @@ def fast_predict(
 
     # trim both raw and scaled predictions if requested
     if trim:
+        trim_args_error_message = (
+            "When 'trim' is set to ``True`` and no explicit values are provided, the "
+            "'trim_min' and 'trim_max' modeler attributes must be present and not "
+            "``None``."
+        )
         default_trim_tolerance = 0.4998
         if trim_tolerance is None:
             try:
                 trim_tolerance = modeler.trim_tolerance
+                if trim_tolerance is None:
+                    trim_tolerance = default_trim_tolerance
             except AttributeError:
                 trim_tolerance = default_trim_tolerance
         try:
@@ -254,11 +276,10 @@ def fast_predict(
                 trim_min = modeler.trim_min
             if trim_max is None:
                 trim_max = modeler.trim_max
+            if any(arg is None for arg in [trim_tolerance, trim_min, trim_max]):
+                raise ValueError(trim_args_error_message) from None
         except AttributeError:
-            raise ValueError(
-                "The trim_min/trim_max modeler attributes must exist when trim=True "
-                "and these values are not explicitly specified"
-            ) from None
+            raise ValueError(trim_args_error_message) from None
         for column in df_predictions.columns:
             df_predictions[f"{column}_trim"] = preprocessor.trim(
                 df_predictions[column], trim_min, trim_max, trim_tolerance


Test patch:
diff --git a/tests/test_experiment_rsmpredict.py b/tests/test_experiment_rsmpredict.py
index f93f00e4..da659800 100644
--- a/tests/test_experiment_rsmpredict.py
+++ b/tests/test_experiment_rsmpredict.py
@@ -287,8 +287,7 @@ class TestExperimentRsmpredict(unittest.TestCase):
                 kwargs["trim_min"] = int(params["trim_min"])
                 kwargs["trim_max"] = int(params["trim_max"])
                 if use_default_trim_tolerance:
-                    if getattr(modeler, "trim_tolerance"):
-                        delattr(modeler, "trim_tolerance")
+                    delattr(modeler, "trim_tolerance")
                 elif "trim_tolerance" in params:
                     kwargs["trim_tolerance"] = float(params["trim_tolerance"])
 
@@ -378,8 +377,8 @@ class TestExperimentRsmpredict(unittest.TestCase):
         with self.assertRaises(ValueError):
             _ = fast_predict(input_features, modeler, df_feature_info)
 
-    def test_fast_predict_no_feature_info(self):
-        """Check case where there is no feature information."""
+    def test_fast_predict_missing_feature_info(self):
+        """Check case where feature information is missing."""
         existing_experiment_dir = join(
             rsmtool_test_dir,
             "data",
@@ -408,6 +407,35 @@ class TestExperimentRsmpredict(unittest.TestCase):
         with self.assertRaises(ValueError):
             _ = fast_predict(input_features, modeler)
 
+    def test_fast_predict_feature_info_none(self):
+        """Check case where feature information is ``None``."""
+        existing_experiment_dir = join(
+            rsmtool_test_dir,
+            "data",
+            "experiments",
+            "lr-predict",
+            "existing_experiment",
+            "output",
+        )
+        model_file = join(existing_experiment_dir, "lr.model")
+
+        # initialize the modeler instance with a bare learner, in which
+        # case no feature_info attribute will exist
+        modeler = Modeler.load_from_learner(Modeler.load_from_file(model_file).learner)
+
+        input_features = {
+            "FEATURE1": 6.0,
+            "FEATURE2": 1.0,
+            "FEATURE3": -0.2,
+            "FEATURE4": 0,
+            "FEATURE5": -0.1,
+            "FEATURE6": 5.0,
+            "FEATURE7": 12,
+            "FEATURE8": -7000,
+        }
+        with self.assertRaises(ValueError):
+            _ = fast_predict(input_features, modeler)
+
     def test_fast_predict_scaling_params_but_scale_false(self):
         """Check case when scaling is turned off yet scaling-related parameters are used."""
         existing_experiment_dir = join(
@@ -478,6 +506,43 @@ class TestExperimentRsmpredict(unittest.TestCase):
                     scale=True,
                 )
 
+    def test_fast_predict_scale_true_scale_attributes_none(self):
+        """Check case when scaling is turned on yet scaling-related parameters are ``None``."""
+        # Load existing model, save its feature info attribute (for later
+        # use), then reload from its learner attribute to mimic loading
+        # from a bare SKLL learner
+        existing_experiment_dir = join(
+            rsmtool_test_dir,
+            "data",
+            "experiments",
+            "lr-predict",
+            "existing_experiment",
+            "output",
+        )
+        modeler = Modeler.load_from_file(join(existing_experiment_dir, "lr.model"))
+        feature_info = modeler.feature_info
+        input_features = {
+            "FEATURE1": 6.0,
+            "FEATURE2": 1.0,
+            "FEATURE3": -0.2,
+            "FEATURE4": 0,
+            "FEATURE5": -0.1,
+            "FEATURE6": 5.0,
+            "FEATURE7": 12,
+            "FEATURE8": -7000,
+        }
+        modeler = Modeler.load_from_learner(modeler.learner)
+
+        # Make prediction with scaling even though scale-related
+        # attributes will have None values
+        with self.assertRaises(ValueError):
+            _ = fast_predict(
+                input_features,
+                modeler,
+                df_feature_info=feature_info,
+                scale=True,
+            )
+
     def test_fast_predict_trimming_params_but_trim_false(self):
         """Check case when trimming is turned off yet trimming-related parameters are used."""
         existing_experiment_dir = join(
@@ -547,3 +612,40 @@ class TestExperimentRsmpredict(unittest.TestCase):
                     modeler,
                     trim=True,
                 )
+
+    def test_fast_predict_trim_true_trim_attributes_none(self):
+        """Check case when trimming is turned on yet trimming-related parameters are ``None``."""
+        # Load existing model, save its feature info attribute (for later
+        # use), then reload from its learner attribute to mimic loading
+        # from a bare SKLL learner
+        existing_experiment_dir = join(
+            rsmtool_test_dir,
+            "data",
+            "experiments",
+            "lr-predict",
+            "existing_experiment",
+            "output",
+        )
+        modeler = Modeler.load_from_file(join(existing_experiment_dir, "lr.model"))
+        feature_info = modeler.feature_info
+        input_features = {
+            "FEATURE1": 6.0,
+            "FEATURE2": 1.0,
+            "FEATURE3": -0.2,
+            "FEATURE4": 0,
+            "FEATURE5": -0.1,
+            "FEATURE6": 5.0,
+            "FEATURE7": 12,
+            "FEATURE8": -7000,
+        }
+        modeler = Modeler.load_from_learner(modeler.learner)
+
+        # Make prediction with trimming even though trim-related
+        # attributes will have None values
+        with self.assertRaises(ValueError):
+            _ = fast_predict(
+                input_features,
+                modeler,
+                df_feature_info=feature_info,
+                trim=True,
+            )