[MRG] Determine IsotonicRegression `increasing` by Pearson or Spearman corr rho #3157

mjbommar · 2014-05-18T14:37:00Z

Sorry to mix a bit of docstring fix with a real PR, but:

Adding the missing increasing docstring to IsotonicRegression. Missing here: http://scikit-learn.org/stable/modules/generated/sklearn.isotonic.IsotonicRegression.html#sklearn.isotonic.IsotonicRegression
Adding the option to use either scipy.stats.pearsonr or scipy.stats.spearmanr to estimate whether increasing should be True or False. Tests included and passing for isotonic, though there appears to be an issue with sklearn.utils.tests.test_sparsefuncs.test_mean_variance_axis0 for me at the moment when I merged to upstream earlier this morning.

…otonicRegression.

…increasing argument options

coveralls · 2014-05-18T14:46:19Z

Coverage decreased (-0.01%) when pulling 031e5e7 on mjbommar:isotonic-increasing-auto into 974fb95 on scikit-learn:master.

agramfort · 2014-05-18T19:57:00Z

sklearn/isotonic.py

+
+        if self.increasing == 'pearson':
+            # Calculate Pearson rho estimate and set accordingly
+            rho, p_val = pearsonr(X, y)


replace p_val by _ as you don't need it

mjbommar · 2014-05-20T10:56:29Z

@agramfort, refactored as requested and PEP8ed.

ogrisel · 2014-05-20T11:16:25Z

I relaunched the travis test to check that the failure is unrelated.

agramfort · 2014-05-20T11:18:54Z

+1 for merge if tests pass

NelleV · 2014-05-20T11:21:58Z

sklearn/isotonic.py

@@ -113,6 +114,15 @@ class IsotonicRegression(BaseEstimator, TransformerMixin, RegressorMixin):
    y_max : optional, default: None
        If not None, set the highest value of the fit to y_max.

+    increasing : optional, boolean or string, default : True


nitpick, but the numpydoc convention places the type of the parameter, before the fact it is optional, not after.

ogrisel · 2014-05-20T11:23:35Z

What about setting increasing='pearson' by default to be more user-friendly?

NelleV · 2014-05-20T11:38:03Z

This patch looks good to me, apart from my two remarks.

@ogrisel it would make more sense to use spearman correlation here. In fact, I don't there is any cases where you'd want to use pearson correlation instead of spearman for such a task.

mjbommar · 2014-05-20T11:48:31Z

@NelleV, fixed both docstring issues.

@ogrisel, what if I suggested as next steps that we implement a Fisher transformation to determine confidence intervals and raise an exception if the confidence interval spans zero? Would you be OK leaving the _check_increasing method factored this way so as to make this an easy next step? Fisher transformation CI is valid for both Pearson and Spearman.

If we do change the default behavior to one of these approaches, I agree with @NelleV that Spearman should be the default choice.

ogrisel · 2014-05-20T14:01:33Z

If Pearson does not make sense here I would vote for:

increasing in {True, False, "auto" (default)} where "auto" means using Spearman. If 0 is in the 99% CI I would just issue a warning rather than raise an exception.

GaelVaroquaux · 2014-05-20T15:31:39Z

increasing in {True, False, "auto" (default)} where auto means using Spearman. If 0 is in the 99 CI I would just issue a warning rather than raise an exception.

+1

…auto'; implement Fisher transform and warning on 0 \in CI

mjbommar · 2014-05-21T00:11:33Z

OK, @ogrisel and @GaelVaroquaux, done as requested. Also added a check on the CI warning getting raised.

coveralls · 2014-05-21T00:17:34Z

Coverage increased (+0.01%) when pulling e730a6d on mjbommar:isotonic-increasing-auto into 974fb95 on scikit-learn:master.

coveralls · 2014-05-21T00:29:14Z

Coverage increased (+0.01%) when pulling e730a6d on mjbommar:isotonic-increasing-auto into 974fb95 on scikit-learn:master.

jnothman · 2014-05-21T00:59:02Z

sklearn/isotonic.py

+        If boolean, whether or not to fit the isotonic regression with y
+        increasing or decreasing.
+
+        If string and set to "auto," determine whether y should


the comma shouldn't be inside the quotes. You could just say: "auto" determines whether ...

ogrisel · 2014-05-21T08:24:00Z

sklearn/tests/test_isotonic.py

+    x = np.arange(len(y))
+
+    y_ = IsotonicRegression(increasing='auto').fit_transform(
+        x, y)


Could you please add a check that no warning is raised in that case?

OK, added the context handler

I am somewhat confused: I don't see it on the diff on github.

GaelVaroquaux · 2014-05-22T12:44:47Z

sklearn/tests/test_isotonic.py

+        is_increasing = check_increasing(X, y)
+        assert_equal(is_increasing, False)
+        assert_equal(len(w), 1)
+        assert_equal(True, "interval" in str(w[-1].message))


You should be using assert_in (nose.tools.assert_in) here.

Actually, assert_warns_message supports checking substrings in the warning message, e.g.,:

# Check that we got increasing=False and CI warning is_increasing = assert_warns_message(UserWarning, "interval", check_increasing, x, y)

GaelVaroquaux · 2014-05-22T12:47:07Z

Hey!

Thanks for all your efforts. We are almost there.

All these little details make the code of scikit-learn better, and that why we all love it!

mjbommar · 2014-05-22T17:31:15Z

No worries. Just trying to scratch a personal itch as quickly as possible, so appreciate the counter force for quality.

Mind chatting on #scikit-learn/DM sometime to iterate a bit more quickly? Handle is mjbommar

GaelVaroquaux · 2014-05-22T17:35:32Z

I'd rather not use IM. I do a lot of things in parallel and it is very hard for me to keep track of everything. Github's interface is great for that.

-------- Original message --------

From: Michael Bommarito notifications@github.com

Date:22/05/2014 19:31 (GMT+01:00)

To: scikit-learn/scikit-learn scikit-learn@noreply.github.com

Cc: Gael Varoquaux gael.varoquaux@normalesup.org

Subject: Re: [scikit-learn] [MRG] Determine IsotonicRegression `increasing` by Pearson or Spearman corr rho (#3157)

No worries. Just trying to scratch a personal itch as quickly as possible, so appreciate the counter force for quality.

Mind chatting on #scikit-learn/DM sometime to iterate a bit more quickly? Handle is mjbommar

—
Reply to this email directly or view it on GitHub.

mjbommar · 2014-05-24T13:05:25Z

OK, @GaelVaroquaux, I think I've addressed the outstanding issues.

Would you like to include a rework to the isotonic regression example in this PR or consider it separately?
http://scikit-learn.org/dev/auto_examples/plot_isotonic_regression.html

agramfort · 2014-05-24T13:08:12Z

doc/modules/classes.rst

+   :toctree: generated
+   :template: function.rst
+
+   isotonic.check_increasing


don't create a new section just add isotonic.check_increasing below isotonic.isotonic_regression

OK. Are there notes on doc practices or is it mostly manual + make doc to test?

OK. Are there notes on doc practices or is it mostly manual + make doc to
test?

manual ie copy is the way it's done with similar code AFAIK

cd sklearn
make test-doc
cd doc
make html

coveralls · 2014-05-24T13:08:36Z

Coverage increased (+0.01%) when pulling 3fe9641 on mjbommar:isotonic-increasing-auto into 974fb95 on scikit-learn:master.

coveralls · 2014-05-24T13:15:36Z

Coverage increased (+0.01%) when pulling 3fe9641 on mjbommar:isotonic-increasing-auto into 974fb95 on scikit-learn:master.

mjbommar · 2014-05-24T19:21:36Z

Looks like a spurious failure in sklearn.tests.test_common.test_regressor_pickle.

======================================================================
ERROR: sklearn.tests.test_common.test_regressor_pickle('OrthogonalMatchingPursuitCV', <class 'sklearn.linear_model.omp.OrthogonalMatchingPursuitCV'>, array([[-0.44836249, -0.47282444, -1.20608008, ..., -0.75500806,
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_common.py", line 890, in check_regressors_pickle
    regressor.fit(X, y_)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/linear_model/omp.py", line 878, in fit
    omp.fit(X, y)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/linear_model/omp.py", line 695, in fit
    copy_Gram, True).T
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/linear_model/omp.py", line 483, in orthogonal_mp_gram
    return_path=return_path)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/linear_model/omp.py", line 221, in _gram_omp
    **solve_triangular_args)
  File "/usr/lib/python2.7/dist-packages/scipy/linalg/basic.py", line 115, in solve_triangular
    a1, b1 = map(asarray_chkfinite,(a,b))
  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/numpy/lib/function_base.py", line 595, in asarray_chkfinite
    "array must not contain infs or NaNs")
ValueError: array must not contain infs or NaNs

agramfort · 2014-05-25T10:09:12Z

yes it's unrelated

agramfort · 2014-05-25T10:09:40Z

sklearn/isotonic.py

+    # Run Fisher transform to get the rho CI, but handle rho=+/-1
+    if rho not in [-1.0, 1.0]:
+        F = 0.5 * np.log((1 + rho) / (1 - rho))
+        F_se = 1 / np.sqrt(len(x) - 3)


nitpick for floats use math.sqrt and math.log not numpy.

agramfort · 2014-05-25T10:11:28Z

besides +1 for merge

GaelVaroquaux · 2014-05-25T15:52:06Z

@agramfort gave his 👍 I am merging. Thanks a lot @mjbommar . Excellent work.

[MRG] Determine IsotonicRegression ``increasing`` by Pearson or Spearman corr rho

amueller · 2016-02-12T19:26:41Z

hm what was the reason to catch the warnings? #6332 errors because numpy became more strict, and now spearmanr errors. Scipy master has a fix but we need to work around that probably?

ogrisel · 2016-02-15T11:17:10Z

sklearn/isotonic.py

+    if rho >= 0:
+        increasing_bool = True
+    else:
+        increasing_bool = False


This should have been written:

increasing_bool = rho >= 0

Also the variable should have been named just increasing. There is no need to put the expected type of the variable in the variable name.

@ogrisel, if you check below, you can see that the user provides an input increasing which may be either string or True/False. If a string is provided, the proper method is applied to determine the direction, thereby setting increasing_bool. Agreed on other comments but just want to make sure we see the reason re: increasing vs. increasing_bool

Alright, I missed that.

mjbommar added 2 commits May 18, 2014 10:32

Adding string={pearson, spearman} option to increasing argument in Is…

6469483

…otonicRegression.

Adding increasing and decreasing tests for both Pearson and Spearman …

031e5e7

…increasing argument options

mjbommar changed the title ~~Isotonic increasing auto~~ Determine IsotonicRegression increasing by Pearson or Spearman corr rho May 18, 2014

agramfort reviewed May 18, 2014
View reviewed changes

mjbommar added 3 commits May 20, 2014 06:50

Refactoring increasing_bool set into _check_increasing method

42920cd

PEP8ing isotonic tests

fb9357e

PEPing isotonic regression

5572775

NelleV reviewed May 20, 2014
View reviewed changes

Docstring style changes

d565386

mjbommar added 4 commits May 20, 2014 20:02

Change arguments to increasing={'auto', True, False} and default to '…

3d04309

…auto'; implement Fisher transform and warning on 0 \in CI

Adding test for CI check and removing Spearman/Pearson-specific tests.

1b2a194

Minor docstring cleanup

6eb98d9

Minor docstring cleanup

e730a6d

jnothman reviewed May 21, 2014
View reviewed changes

Docstring fix

f63e42e

ogrisel reviewed May 21, 2014
View reviewed changes

GaelVaroquaux reviewed May 22, 2014
View reviewed changes

mjbommar added 6 commits May 22, 2014 14:50

Improving tests based on feedback from @GaelVaroquaux

4c3e7a9

Improving tests based on feedback from @GaelVaroquaux

61a89ba

Fixing matrix vs. vector notation for X

01d4bd2

Fixing space in docstring for default value

73afdf1

Adding check_increasing to classes.rst

16bf2f4

Adding no-warning assertions to IR auto tests

3fe9641

agramfort reviewed May 24, 2014
View reviewed changes

Fixing classes.rst

4a4d8e7

agramfort reviewed May 25, 2014
View reviewed changes

Switching from np to math for scalar float ops

9094078

GaelVaroquaux added a commit that referenced this pull request May 25, 2014

Merge pull request #3157 from mjbommar/isotonic-increasing-auto

5cef947

[MRG] Determine IsotonicRegression ``increasing`` by Pearson or Spearman corr rho

GaelVaroquaux merged commit 5cef947 into scikit-learn:master May 25, 2014

ogrisel reviewed Feb 15, 2016
View reviewed changes

Uh oh!

[MRG] Determine IsotonicRegression increasing by Pearson or Spearman corr rho #3157

[MRG] Determine IsotonicRegression increasing by Pearson or Spearman corr rho #3157

Uh oh!

Conversation

mjbommar commented May 18, 2014

Uh oh!

coveralls commented May 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjbommar commented May 20, 2014

Uh oh!

ogrisel commented May 20, 2014

Uh oh!

agramfort commented May 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented May 20, 2014

Uh oh!

NelleV commented May 20, 2014

Uh oh!

mjbommar commented May 20, 2014

Uh oh!

ogrisel commented May 20, 2014

Uh oh!

GaelVaroquaux commented May 20, 2014

Uh oh!

mjbommar commented May 21, 2014

Uh oh!

coveralls commented May 21, 2014

Uh oh!

coveralls commented May 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented May 22, 2014

Uh oh!

mjbommar commented May 22, 2014

Uh oh!

GaelVaroquaux commented May 22, 2014

Uh oh!

mjbommar commented May 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 24, 2014

Uh oh!

coveralls commented May 24, 2014

Uh oh!

mjbommar commented May 24, 2014

Uh oh!

agramfort commented May 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented May 25, 2014

Uh oh!

GaelVaroquaux commented May 25, 2014

Uh oh!

amueller commented Feb 12, 2016

[MRG] Determine IsotonicRegression `increasing` by Pearson or Spearman corr rho #3157

[MRG] Determine IsotonicRegression `increasing` by Pearson or Spearman corr rho #3157