Repository: UBC-MDS/PyPunisher

Issue:
Feedback on Milestone 2
Hi All,

Nice work for milestone 2. I like your comprehensive designs for the entire package. Here is my comments:

1. Good Practice to state out installation requires python 3.6

2. I like your coverage part to detail your test coverage, excellent

3. For __init__.py, line 4 and 5, why not just list.append() to have version number?

4. For selection_engines/__init__.py, I like your comments for the issue in scipy

5. Please improve your style in Python programming, you can refer to https://google.github.io/styleguide/pyguide.html  as in selection.py line 4, this is not professional, also the space you have between lines are not equal.

6. For your first line of your python file, it is suggested that you can include #!/usr/bin/env python just in case the user is running your code in Linux(like me)

7. for _fit_and_score(self, S, feature, algorithm), what if algorithm input is wrong input parameters? if you thought about that?

8. For function backward(), S is not a good naming for a list

Regards
Jason

Discussion:

Patch:
diff --git a/pypunisher/__init__.py b/pypunisher/__init__.py
index 97d4631..0730e06 100644
--- a/pypunisher/__init__.py
+++ b/pypunisher/__init__.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+
 """
 PyPunisher
 ==========
diff --git a/pypunisher/_checks.py b/pypunisher/_checks.py
index a268a99..ad8a08f 100644
--- a/pypunisher/_checks.py
+++ b/pypunisher/_checks.py
@@ -1,8 +1,8 @@
-"""
-
-    Checks
-    ~~~~~~
+#!/usr/bin/env python
 
+"""
+Checks
+======
 """
 import numpy as np
 
@@ -48,20 +48,21 @@ def input_checks(locals_):
     """
     # Sort so that the order of the parameter name
     # are in a reliable (alphabetical) order.
-    param_a, param_b = sorted(k for k, p in locals_.items() if k != 'self')
-    locals_non_non = {k: v for k, v in locals_.items()
-                      if v is not None and k != 'self'}
+    ALLOWED = ('min_change', 'n_features')
+    param_a, param_b = sorted(k for k, p in locals_.items() if k in ALLOWED)
+    locals_non_none = {k: v for k, v in locals_.items()
+                       if v is not None and k in ALLOWED}
 
-    if len(locals_non_non) != 1:
+    if len(locals_non_none) != 1:
         raise TypeError(
             "At least one of `{a}` and `{b}` must be None.".format(
                 a=param_a, b=param_b
             )
         )
 
-    # Unpack the single key and value pair
-    name, obj = tuple(locals_non_non.items())[0]
-    if obj is None and not isinstance(obj, (int, float)):
+    # Unpack the single key and value pair.
+    name, obj = tuple(locals_non_none.items())[0]
+    if not isinstance(obj, (int, float)):
         raise TypeError(
             "`{}` must be of type int or float.".format(name)
         )
diff --git a/pypunisher/metrics/__init__.py b/pypunisher/metrics/__init__.py
index 0ef76a8..8fe54ef 100644
--- a/pypunisher/metrics/__init__.py
+++ b/pypunisher/metrics/__init__.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+
 """
 Metrics
 =======
diff --git a/pypunisher/metrics/criterion.py b/pypunisher/metrics/criterion.py
index 91c5150..70e599b 100644
--- a/pypunisher/metrics/criterion.py
+++ b/pypunisher/metrics/criterion.py
@@ -1,8 +1,8 @@
-"""
-
-     Information Criterion
-     ~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+ Information Criterion
+ =====================
 """
 from numpy import log, ndarray, pi
 from pypunisher._checks import model_check
diff --git a/pypunisher/selection_engines/__init__.py b/pypunisher/selection_engines/__init__.py
index 65479f8..aac2dd9 100644
--- a/pypunisher/selection_engines/__init__.py
+++ b/pypunisher/selection_engines/__init__.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+
 """
 Selection Engines
 =================
diff --git a/pypunisher/selection_engines/_utils.py b/pypunisher/selection_engines/_utils.py
index 9315f08..013198a 100644
--- a/pypunisher/selection_engines/_utils.py
+++ b/pypunisher/selection_engines/_utils.py
@@ -1,8 +1,8 @@
-"""
-
-    Utils
-    ~~~~~
+#!/usr/bin/env python
 
+"""
+Utils
+=====
 """
 
 def get_n_features(matrix, min_=2):
@@ -47,8 +47,7 @@ def enforce_use_of_all_cpus(model):
             exists
 
     """
-    if hasattr(model, 'n_jobs'):
-        setattr(model, 'n_jobs', -1)
+    setattr(model, 'n_jobs', -1)
     return model
 
 
diff --git a/pypunisher/selection_engines/selection.py b/pypunisher/selection_engines/selection.py
index 067a3f5..561aec7 100644
--- a/pypunisher/selection_engines/selection.py
+++ b/pypunisher/selection_engines/selection.py
@@ -1,8 +1,8 @@
-"""
-
-    Forward and Backward Selection Algorithms
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Forward and Backward Selection Algorithms
+=========================================
 """
 from pypunisher.metrics.criterion import aic, bic
 from pypunisher._checks import model_check, array_check, input_checks
@@ -93,6 +93,27 @@ class Selection(object):
             score = self._model.score(X_val, y_val)
         return score
 
+    @staticmethod
+    def _do_not_skip(kwargs):
+        """Check for skipping override by looking
+        for `_do_not_skip` in keyword arguments
+        If it is present, the loops in the algorithms
+        will be run to exhaustion.
+
+        Args:
+            kwargs : dict
+                Keyword Args
+
+        Returns:
+            Bool
+                If `_do_not_skip` is not present
+                      or `_do_not_skip` is present and is True.
+                Otherwise, the value of `do_not_skip`
+                      is returned.
+
+        """
+        return kwargs.get('_do_not_skip', True)
+
     def _forward_break_criteria(self, S, min_change, best_j_score,
                                 j_score_dict, n_features):
         """Check if `forward()` should break
@@ -128,7 +149,7 @@ class Selection(object):
         else:
             return False
 
-    def forward(self, min_change=0.5, n_features=None):
+    def forward(self, min_change=0.5, n_features=None, **kwargs):
         """Perform Forward Selection on a Sklearn model.
 
         Args:
@@ -140,6 +161,10 @@ class Selection(object):
                 Note: `min_change` must be None in order for `n_features` to operate.
                 Floats will be regarded as proportions of the total
                 that must lie on (0, 1).
+            kwargs : Keyword Args
+                Includes:
+                 * `_do_not_skip`: for interal use only; it is
+                    not recommended that users use this parameter.
 
         Returns:
             S : list
@@ -150,14 +175,18 @@ class Selection(object):
         S = list()
         best_score = None
         itera = list(range(self._total_number_of_features))
+        do_not_skip = self._do_not_skip(kwargs)
 
-        if n_features:
+        if n_features and do_not_skip:
             n_features = parse_n_features(n_features, total=len(itera))
 
         for i in range(self._total_number_of_features):
             if self._verbose:
                 print("Iteration: {}".format(i))
 
+            if not do_not_skip:
+                continue
+
             # 1. Find best feature, j, to add.
             j_score_dict = dict()
             for j in itera:
@@ -182,7 +211,7 @@ class Selection(object):
 
         return S
 
-    def backward(self, n_features=0.5, min_change=None):
+    def backward(self, n_features=0.5, min_change=None, **kwargs):
         """Perform Backward Selection on a Sklearn model.
 
         Args:
@@ -194,6 +223,14 @@ class Selection(object):
             min_change : int or float, optional
                 The smallest change to be considered significant.
                 `n_features` must be None for `min_change` to operate.
+            kwargs : Keyword Args
+                Includes:
+                 * `_do_not_skip` : bool
+                    Explore loop exhaustion.
+                    **For internal use only**; Not intended for outside use.
+                * `_last_score_punt` : bool
+                    Relax `defeated_last_iter_score` decision boundary.
+                    **For internal use only**. Not intended for outside use.
 
         Returns:
             S : list
@@ -205,8 +242,10 @@ class Selection(object):
         """
         input_checks(locals())
         S = list(range(self._total_number_of_features))  # start with all features
+        do_not_skip = self._do_not_skip(kwargs)
+        last_score_punt = kwargs.get('_last_score_punt', False)
 
-        if n_features:
+        if n_features and do_not_skip:
             n_features = parse_n_features(n_features, total=len(S))
 
         last_iter_score = self._fit_and_score(S, feature=None, algorithm='backward')
@@ -215,6 +254,9 @@ class Selection(object):
             if self._verbose:
                 print("Iteration: {}".format(i))
 
+            if not do_not_skip:
+                continue
+
             # 1. Hunt for the least predictive feature.
             best = {'feature': None, 'score': None, 'defeated_last_iter_score': True}
             for j in S:
@@ -228,13 +270,13 @@ class Selection(object):
             if isinstance(n_features, int):
                 S.remove(to_drop)  # blindly drop.
                 last_iter_score = best_new_score
-                if len(S) == n_features:
-                    break
-                else:
+                if not len(S) == n_features:
                     continue  # i.e., ignore criteria below.
+                else:
+                    break
             # 2b. Halt if the change is not longer considered significant.
-            if isinstance(min_change, (int, float)):
-                if best['defeated_last_iter_score']:
+            else:
+                if best['defeated_last_iter_score'] or last_score_punt:
                     if (best_new_score - last_iter_score) < min_change:
                         break  # there was a change, but it was not large enough.
                     else:
@@ -243,8 +285,4 @@ class Selection(object):
                 else:
                     break
 
-            # 2c. Halt if only one feature remains.
-            if len(S) == 1:
-                break
-
         return S


Test patch:
diff --git a/tests/__init__.py b/tests/__init__.py
index e69de29..4265cc3 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -0,0 +1,1 @@
+#!/usr/bin/env python
diff --git a/tests/_defaults.py b/tests/_defaults.py
index b9a31c4..ddfc126 100644
--- a/tests/_defaults.py
+++ b/tests/_defaults.py
@@ -1,8 +1,8 @@
-"""
+#!/usr/bin/env python
 
+"""
     Default Base for Testing Against
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
+    ================================
 """
 from sklearn.linear_model import LinearRegression
 from tests._test_data import X_train, y_train, X_val, y_val
diff --git a/tests/_test_data.py b/tests/_test_data.py
index 5b74671..adc5f13 100644
--- a/tests/_test_data.py
+++ b/tests/_test_data.py
@@ -1,31 +1,41 @@
-"""
-
-    Test Data
-    ~~~~~~~~~
-
-    Generate: y = x + e, where e ~ Uniform(0, 50) and
-    `x` is embedded as the middle column in a zero matrix.
-    That is, only ONE column is predictive of y, the rest are
-    trivial column vectors.
+#!/usr/bin/env python
 
+"""
+Test Data
+=========
+Generate: y = x + e, where e ~ Uniform(0, 50) and
+`x` is embedded as the middle column in a zero matrix.
+That is, only ONE column is predictive of y, the rest are
+trivial column vectors.
+
+X_train : 2D array
+    Training Features.
+X_val : 2D array
+    Validation Features.
+y_train : 1D array
+    Training labels.
+y_val : 1D array
+    Validation Labels
+true_best_feature : int, list
+    Denotes the best feature
+    that is actually predictive of the response.
 """
 import numpy as np
 from sklearn.model_selection import train_test_split
 
 SEED = 99
-
-features = 20
-obs = 501
-middle_feature = features // 2
+FEATURES = 20
+OBSERVATIONS = 501
+middle_feature = FEATURES // 2
 
 np.random.seed(SEED)
-X = np.zeros((obs, features))
-y = np.arange(obs)
-X[:, middle_feature] = y + np.random.uniform(0, 50, size=obs)
+X = np.zeros((OBSERVATIONS, FEATURES))
+y = np.arange(OBSERVATIONS)
+X[:, middle_feature] = y + np.random.uniform(0, 50, size=OBSERVATIONS)
 
 X_train, X_val, y_train, y_val = train_test_split(X, y, random_state=SEED)
 
-TRUE_BEST_FEATURE = middle_feature
+true_best_feature = middle_feature
 
 # Visualize ---
 # import matplotlib.pyplot as plt
diff --git a/tests/_wrappers.py b/tests/_wrappers.py
index e4404f9..2dcf69d 100644
--- a/tests/_wrappers.py
+++ b/tests/_wrappers.py
@@ -1,8 +1,8 @@
-"""
-
-    Wrapper Functions for Testing
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Wrapper Functions for Testing
+=============================
 """
 from copy import deepcopy
 from tests._defaults import DEFAULT_SELECTION_PARAMS
diff --git a/tests/test_backward_selection.py b/tests/test_backward_selection.py
index d44f603..cdb8b2a 100644
--- a/tests/test_backward_selection.py
+++ b/tests/test_backward_selection.py
@@ -1,8 +1,8 @@
-"""
-
-    Tests Specific to Backward Selection
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Tests Specific to Backward Selection
+====================================
 """
 import os
 import sys
@@ -13,33 +13,88 @@ sys.path.insert(0, os.path.abspath("."))
 sys.path.insert(0, os.path.abspath("../"))
 
 from tests._wrappers import backward
+from tests._test_data import X_train
+
 
+# -----------------------------------------------------------------------------
+# Test `backward()` Params
+# -----------------------------------------------------------------------------
 
-def test_backward_params():
+
+def test_n_features_greater_than_zero_backward():
     """
-    Check parameters to `backward()` raise when expected.
+    Check `backward()`'s `n_features` raises when
+    not greater than zero
     """
     msg = "`n_features` must be greater than zero."
     with pytest.raises(ValueError, match=msg):
         backward(n_features=-0.5, min_change=None)
 
+
+def test_min_change_greater_than_zero_backward():
+    """
+    Check `backward()`'s `min_change` raises when
+    not greater than zero
+    """
     msg = "`min_change` must be greater than zero."
     with pytest.raises(ValueError, match=msg):
         backward(n_features=None, min_change=-0.75)
 
+
+def test_min_change_fails_on_string_backward():
+    """
+    Check that backward raises when passed a string
+    for `min_change`.
+    """
+    msg = "`min_change` must be of type int or float."
+    with pytest.raises(TypeError, match=msg):
+        backward(min_change='-0.75', n_features=None)
+
+
+def test_n_features_fails_on_string_backward():
+    """
+    Check that backward raises when passed a string
+    for `n_features`.
+    """
+    msg = "`n_features` must be of type int or float."
+    with pytest.raises(TypeError, match=msg):
+        backward(min_change=None, n_features='-0.75')
+
+
+def test_both_non_none_backward():
+    """
+    Check `backward()` raise when at least one
+    of `min_change` or `n_features` are not None.
+    """
     # Note: items in backticks (``) will be in alphabetical order.
     msg = "At least one of `min_change` and `n_features` must be None."
     with pytest.raises(TypeError, match=msg):
         backward(n_features=0.5, min_change=0.3)
-    
-    msg = "`criterion` must be one of: None, 'aic', 'bic'."
-    with pytest.raises(ValueError, match=msg):
-        backward(n_features=0.5, criterion='acc')
 
+
+def test_float_greater_than_one_raises_backward():
+    """
+    Test that float values not on (0, 1) raise.
+    """
     msg = "^If a float, `n_features` must be on"
     with pytest.raises(ValueError, match=msg):
         backward(n_features=1.5)
-    
+
+
+def test_min_features_requirement_backward():
+    """
+    Check that the requirement that at least
+    two features must be present.
+    """
     msg = "less than 2 features present."
     with pytest.raises(IndexError, match=msg):
-        backward(X_train=ones((501, 1)), X_val=ones((501, 1)))
\ No newline at end of file
+        backward(X_train=ones((501, 1)), X_val=ones((501, 1)))
+
+
+# -----------------------------------------------------------------------------
+# Test Exhausting loop
+# -----------------------------------------------------------------------------
+
+def test_loop_exhaust():
+    """Text Exhausting backward()'s loop."""
+    backward(n_features=X_train.shape[-1], min_change=None, _do_not_skip=False)
diff --git a/tests/test_criterion.py b/tests/test_criterion.py
index 9e2b8ff..2773219 100644
--- a/tests/test_criterion.py
+++ b/tests/test_criterion.py
@@ -1,8 +1,8 @@
-"""
-
-    Criterion Tests
-    ~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Criterion Tests
+===============
 """
 import os
 import sys
@@ -15,6 +15,7 @@ import statsmodels.api as sm
 from pypunisher.metrics.criterion import aic, bic
 from sklearn.linear_model import LinearRegression
 from tests._test_data import X_train, y_train
+from tests._wrappers import forward, backward
 
 COMP_TOLERANCE = 200  # comparision tolerance between floats
 
@@ -49,6 +50,22 @@ def test_metric_model_param():
                 metric(kind, X_train=X_train, y_train=y_train)
 
 
+# -----------------------------------------------------------------------------
+# Test criterion through selection
+# -----------------------------------------------------------------------------
+
+
+def test_selection_class_use_of_criterion():
+    """Test Criterion through `forward()` and `backward()."""
+
+    msg = "`criterion` must be one of: None, 'aic', 'bic'."
+    with pytest.raises(ValueError, match=msg):
+        forward(min_change=0.5, criterion='acc')
+
+    with pytest.raises(ValueError, match=msg):
+        backward(n_features=0.5, criterion='Santa')
+
+
 # -----------------------------------------------------------------------------
 # `data` Param
 # -----------------------------------------------------------------------------
@@ -68,6 +85,7 @@ def test_metric_data_param():
             else:
                 metric(sk_model, X_train=kind, y_train=y_train)
 
+
 # -----------------------------------------------------------------------------
 # Metric output
 # -----------------------------------------------------------------------------
diff --git a/tests/test_forward_selection.py b/tests/test_forward_selection.py
index fa62b01..93d27f0 100644
--- a/tests/test_forward_selection.py
+++ b/tests/test_forward_selection.py
@@ -1,8 +1,8 @@
-"""
-
-    Tests Specific to Forward Selection
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Tests Specific to Forward Selection
+===================================
 """
 import os
 import sys
@@ -12,25 +12,68 @@ sys.path.insert(0, os.path.abspath("."))
 sys.path.insert(0, os.path.abspath("../"))
 
 from tests._wrappers import forward
+from tests._test_data import X_train
+
+
+# -----------------------------------------------------------------------------
+# Test `forward()` Params
+# -----------------------------------------------------------------------------
+
+
+def test_n_features_greater_than_zero_forward():
+    """
+    Check that `n_features` is required to be > 0.
+    """
+    msg = "`n_features` must be greater than zero."
+    with pytest.raises(ValueError, match=msg):
+        forward(min_change=None, n_features=-0.75)
 
 
-def test_forward_params():
+def test_min_change_greater_than_zero_forward():
     """
-    Check parameters to `forward()` raise when expected.
+    Check that `min_change` is required to be > 0.
     """
     msg = "`min_change` must be greater than zero."
     with pytest.raises(ValueError, match=msg):
         forward(min_change=-0.5, n_features=None)
 
-    msg = "`n_features` must be greater than zero."
-    with pytest.raises(ValueError, match=msg):
-        forward(min_change=None, n_features=-0.75)
 
+def test_n_features_fails_on_string_forward():
+    """
+    Check that forward raises when passed a string
+    for `n_features`.
+    """
+    msg = "`n_features` must be of type int or float."
+    with pytest.raises(TypeError, match=msg):
+        forward(min_change=None, n_features='-0.75')
+
+
+def test_min_change_fails_on_string_forward():
+    """
+    Check that forward raises when passed a string
+    for `min_change`.
+    """
+    msg = "`min_change` must be of type int or float."
+    with pytest.raises(TypeError, match=msg):
+        forward(min_change='-0.75', n_features=None)
+
+
+def test_both_non_none_forward():
+    """
+    Check `forward()` raise when at least one
+    of `min_change` or `n_features` are not None.
+    """
     # Note: items in backticks (``) will be in alphabetical order.
     msg = "At least one of `min_change` and `n_features` must be None."
     with pytest.raises(TypeError, match=msg):
         forward(min_change=0.5, n_features=0.3)
-    
-    msg = "`criterion` must be one of: None, 'aic', 'bic'."
-    with pytest.raises(ValueError, match=msg):
-        forward(min_change=0.5, criterion='acc')
+
+
+# -----------------------------------------------------------------------------
+# Test Exhausting loop
+# -----------------------------------------------------------------------------
+
+def test_loop_exhaust():
+    """Text Exhausting forwards()'s loop."""
+    # Should not raise.
+    forward(n_features=X_train.shape[-1], min_change=None, _do_not_skip=False)
diff --git a/tests/test_selection.py b/tests/test_selection.py
index e3ee45a..2eb6098 100644
--- a/tests/test_selection.py
+++ b/tests/test_selection.py
@@ -1,8 +1,8 @@
-"""
-
-    Run Tests Common to Forward and Backward Selection
-    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+#!/usr/bin/env python
 
+"""
+Run Tests Common to Forward and Backward Selection
+==================================================
 """
 import os
 import sys
@@ -14,7 +14,7 @@ sys.path.insert(0, os.path.abspath("../"))
 
 from pypunisher import Selection
 from tests._wrappers import forward, backward
-from tests._test_data import TRUE_BEST_FEATURE
+from tests._test_data import true_best_feature
 from tests._defaults import DEFAULT_SELECTION_PARAMS
 
 
@@ -75,6 +75,18 @@ def test_sklearn_model_methods():
         with pytest.raises(AttributeError):
             Selection(**d)
 
+# -----------------------------------------------------------------------------
+# Test Multiple Features
+# -----------------------------------------------------------------------------
+
+
+def test_passing_significant_change():
+    """
+    Test cases where there is a significant `min_change`
+    during backward selection.
+    """
+    backward(n_features=None, min_change=1, _last_score_punt=True)
+
 
 # -----------------------------------------------------------------------------
 # Outputs: Run the Forward and Backward Selection Algorithms
@@ -88,7 +100,7 @@ forward_output += forward(n_features=1, min_change=None)
 # Force the backward selection algorithm to
 # select the single feature it thinks is most predictive.
 # If implemented correctly, `backward()` should be able to
-# identify `TRUE_BEST_FEATURE` as predictive.
+# identify `true_best_feature` as predictive.
 backward_output = backward(n_features=1)
 
 # Run using the other parameter option
@@ -142,7 +154,7 @@ def output_values(output):
     in the contrived data.
     """
     msg = "The algorithm failed to select the predictive feature."
-    assert TRUE_BEST_FEATURE in output, msg
+    assert true_best_feature in output, msg
 
 
 def test_fsel_output_values():
@@ -206,4 +218,4 @@ def test_fsel_verbose_output():
 
 def test_bsel_verbose_output():
     backward_output = backward(n_features=2, min_change=None, verbose=True)
-    assert len(backward_output) >= 1
\ No newline at end of file
+    assert len(backward_output) >= 1