Repository: EducationalTestingService/rsmtool

Issue:
More meaningful error message for non-numeric features
It would be helpful to print a more explicit message, if one of the features only contains non-numeric values specifying the name of the feature. 

This should be done here: 

https://github.com/EducationalTestingService/rsmtool/blob/0eb9a96e236828483123e46a4b1e2f1376c59827/rsmtool/preprocessor.py#L753


Discussion:

Patch:
diff --git a/rsmtool/configuration_parser.py b/rsmtool/configuration_parser.py
index 633c309a..11783b07 100644
--- a/rsmtool/configuration_parser.py
+++ b/rsmtool/configuration_parser.py
@@ -694,10 +694,12 @@ class ConfigurationParser:
         Raises
         ------
         FileNotFoundError
-            If the given config file path does not exist.
+            If the given path does not exist.
+        OSError:
+            If the given path is a directory, not a file.
         ValueError
-            If the configuration file does not have a valid extension.
-            The only valid extension is ``.json``.
+            If the file at the given path does not have
+            a valid extension (``.json``).
         """
         # if we passed in a string, convert it to a Path
         if isinstance(pathlike, str):
@@ -706,14 +708,18 @@ class ConfigurationParser:
         # raise an exception if the file does not exist
         if not pathlike.exists():
             raise FileNotFoundError(f"The configuration file {pathlike} "
-                                    "was not found.")
+                                    f"was not found.")
 
-        # make sure we have a JSON configuration file
+        # raise an exception if the user specified a directory
+        if not pathlike.is_file():
+            raise OSError(f"The given path {pathlike} should be a "
+                          f"file, not a directory.")
+
+        # make sure we have a file that ends in ".json"
         extension = pathlike.suffix.lower()
         if extension != '.json':
-            raise ValueError('Configuration file must be '
-                             'in `.json` '
-                             'format. You specified: {}.'.format(extension))
+            raise ValueError(f"The configuration file must be in `.json` "
+                             f"format. You specified: {extension}.")
 
         # set the various attributes to None
         self._filename = pathlike.name
diff --git a/rsmtool/preprocessor.py b/rsmtool/preprocessor.py
index 3b8cfed8..81fbc7d5 100644
--- a/rsmtool/preprocessor.py
+++ b/rsmtool/preprocessor.py
@@ -740,6 +740,9 @@ class FeaturePreprocessor:
         # create a copy of the original data frame
         df_filter = df.copy()
 
+        # we start out assuming that we will not drop this column
+        drop_column = False
+
         # return a copy of the original data frame if
         # the given column does not exist at all
         if column not in df.columns:
@@ -752,43 +755,54 @@ class FeaturePreprocessor:
 
         # Save the values that have been converted to NaNs
         # as a separate data frame. We want to keep them as NaNs
-        # to do more analyses later.
-        # We also filter out inf values. Since these can only be generated
-        # during transformations we convert them to NaNs for consistency.
-        bad_rows = df_filter[df_filter[column].isnull() | np.isinf(df_filter[column])]
+        # to do more analyses later. We also filter out inf values.
+        # Since these can only be generated during transformations,
+        # we include them with NaNs for consistency.
+        bad_rows = df_filter[column].isnull() | np.isinf(df_filter[column])
+        df_bad_rows = df_filter[bad_rows]
+
+        # if the column contained only non-numeric values, we need to drop it
+        if len(df_bad_rows) == len(df_filter):
+            logging.info(f"Feature {column} was excluded from the model "
+                         f"because it only contains non-numeric values.")
+            drop_column = True
 
-        # drop the NaNs that we might have gotten
-        df_filter = df_filter[df_filter[column].notnull() & ~np.isinf(df_filter[column])]
+        # now drop the above bad rows containing NaNs from our data frame
+        df_filter = df_filter[~bad_rows]
 
         # exclude zeros if specified
         if exclude_zeros:
-            zero_indices = df_filter[df_filter[column] == 0].index.values
-            zero_rows = df.loc[zero_indices]
-            df_filter = df_filter[df_filter[column] != 0]
+            zero_rows = df_filter[column] == 0
+            df_zero_rows = df_filter[zero_rows]
+            df_filter = df_filter[~zero_rows]
         else:
-            zero_rows = pd.DataFrame()
+            df_zero_rows = pd.DataFrame()
 
         # combine all the filtered rows into a single data frame
-        df_exclude = pd.concat([bad_rows, zero_rows], sort=True)
+        df_exclude = pd.concat([df_bad_rows, df_zero_rows], sort=True)
 
         # reset the index so that the indexing works correctly
         # for the next feature with missing values
         df_filter.reset_index(drop=True, inplace=True)
         df_exclude.reset_index(drop=True, inplace=True)
 
-        # Drop this column if the standard deviation equals zero:
+        # Check if the the standard deviation equals zero:
         # for training set sd == 0 will break normalization.
         # We set the tolerance level to the 6th digit
-        # to account for a possibility that the exact value
-        # computed by std is not 0
+        # to account for the possibility that the exact value
+        # computed by `std()` is not 0
         if exclude_zero_sd is True:
             feature_sd = df_filter[column].std()
             if np.isclose(feature_sd, 0, atol=1e-07):
-                logging.info("Feature {} was excluded from the model "
-                             "because its standard deviation in the "
-                             "training set is equal to 0.".format(column))
-                df_filter = df_filter.drop(column, 1)
-                df_exclude = df_exclude.drop(column, 1)
+                logging.info(f"Feature {column} was excluded from the model "
+                             f"because its standard deviation in the "
+                             f"training set is equal to 0.")
+                drop_column = True
+
+        # if `drop_column` is true, then we need to drop the column
+        if drop_column:
+            df_filter = df_filter.drop(column, axis=1)
+            df_exclude = df_exclude.drop(column, axis=1)
 
         # return the filtered rows and the new data frame
         return (df_filter, df_exclude)


Test patch:
diff --git a/tests/test_configuration_parser.py b/tests/test_configuration_parser.py
index d50f0d13..48a81b2e 100644
--- a/tests/test_configuration_parser.py
+++ b/tests/test_configuration_parser.py
@@ -2,7 +2,6 @@ import json
 import logging
 import os
 import tempfile
-import warnings
 import pandas as pd
 
 from io import StringIO
@@ -16,7 +15,6 @@ from pandas.testing import assert_frame_equal
 
 from nose.tools import (assert_equal,
                         assert_not_equal,
-                        assert_raises,
                         eq_,
                         ok_,
                         raises)
@@ -35,6 +33,20 @@ class TestConfigurationParser:
     def setUp(self):
         pass
 
+    @raises(FileNotFoundError)
+    def test_init_nonexistent_file(self):
+        non_existent_file = "/x/y.json"
+        _ = ConfigurationParser(non_existent_file)
+
+    @raises(OSError)
+    def test_init_directory_instead_of_file(self):
+        with tempfile.TemporaryDirectory() as tempd:
+            _ = ConfigurationParser(tempd)
+
+    @raises(ValueError)
+    def test_init_non_json_file(self):
+        with tempfile.NamedTemporaryFile(suffix=".txt") as tempf:
+            _ = ConfigurationParser(tempf.name)
 
     def test_parse_config_from_dict_rsmtool(self):
         data = {'experiment_id': 'experiment_1',
@@ -51,7 +63,6 @@ class TestConfigurationParser:
         assert_equal(newdata['description'], '')
         assert_equal(newdata.configdir, getcwd())
 
-
     def test_parse_config_from_dict_rsmeval(self):
         data = {'experiment_id': 'experiment_1',
                 'predictions_file': 'data/rsmtool_smTrain.csv',
diff --git a/tests/test_preprocessor.py b/tests/test_preprocessor.py
index 1ba15d65..08b67d2c 100644
--- a/tests/test_preprocessor.py
+++ b/tests/test_preprocessor.py
@@ -185,13 +185,14 @@ class TestFeaturePreprocessor:
                                'spkitemlab': range(1, 9)})
 
         expected_df_excluded = bad_df.copy()
-        expected_df_excluded['sc1'] = np.nan
+        expected_df_excluded.drop('sc1', axis=1, inplace=True)
 
         df_filtered, df_excluded = self.fpp.filter_on_column(bad_df, 'sc1',
                                                              'spkitemlab',
                                                              exclude_zeros=True)
 
         ok_(df_filtered.empty)
+        ok_("sc1" not in df_filtered.columns)
         assert_frame_equal(df_excluded, expected_df_excluded, check_dtype=False)
 
     def test_filter_on_column_std_epsilon_zero(self):