[MRG] varimax rotation for factor analysis by jona-sassenhagen · Pull Request #11064 · scikit-learn/scikit-learn

jona-sassenhagen · 2018-05-03T13:30:29Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

As discussed in #2688, sklearn's Factor Analysis does not support rotations. This makes it essentially useless for e.g. psychologists.

This PR implements the varimax and quartimax rotations.

sklearn-lgtm · 2018-05-03T14:11:22Z

This pull request introduces 1 alert when merging 99991b7 into 593bf65 - view on lgtm.com

new alerts:

1 for Syntax error

Comment posted by lgtm.com

jona-sassenhagen · 2018-05-04T12:51:40Z

Well, it's green ...

@jnothman

jnothman

Just looking at the basics, thanks

jnothman · 2018-05-06T01:12:06Z


+    rotation : None | 'varimax' | 'orthomax'
+        If not None, apply the indicated rotation. Currently, varimax and
+        orthomax are implemented.


Should there be a reference or some discussion in the user guide? At least there should be a link to the example from the user guide

jnothman · 2018-05-06T01:16:08Z

+    # test rotation
+    n_comps = 2
+    for method in ("varimax", 'quartimax'):
+        fa_var = FactorAnalysis(n_components=n_comps, rotation=method)


Please assert that the solution varies with the parameter

jona-sassenhagen · 2018-05-06T13:11:56Z

CIs on 9f582f0 can all be canceled, I pushed quite preemptively.

jona-sassenhagen · 2018-05-12T21:23:48Z

@jnothman better?

jnothman · 2018-05-15T11:56:09Z

+        fa_var = FactorAnalysis(n_components=n_comps, rotation=method)
+        results.append(fa_var.fit_transform(X))
+    for a, b in ((0, 1), (0, 2), (1, 2)):
+        assert_raises(AssertionError, assert_array_almost_equal,


I think assert not np.allclose would be clearer

Hm ... only here, or also in 112? 84 also uses assert_array_almost_equal.

You can use assert_allclose in those cases, but here you are raising an error in the case that the assertion fails, which is especially difficult to read.

jona-sassenhagen · 2018-05-30T15:50:03Z

@jnothman now? :)

jona-sassenhagen · 2018-06-02T07:10:38Z

Anything else? It seems ready to me.

jona-sassenhagen · 2018-06-07T09:43:37Z

@jnothman can I bother you about this one again? :)

jnothman · 2018-06-07T23:02:16Z

I don't feel confident to review the technical basis without being able to sit down with references. in that vein, could you please provide references or a technical description of what the rotation optimises in the user guide

jnothman · 2018-05-23T14:29:59Z

+        fa_var = FactorAnalysis(n_components=n_comps, rotation=method)
+        results.append(fa_var.fit_transform(X))
+    for a, b in ((0, 1), (0, 2), (1, 2)):
+        assert_raises(AssertionError, assert_array_almost_equal,


You can use assert_allclose in those cases, but here you are raising an error in the case that the assertion fails, which is especially difficult to read.

jnothman · 2018-06-07T23:03:04Z

    :align: center
    :scale: 75%

+Factor Analysis is often followed by a rotation of the factors, usually to improve


Please note the parameter name to control this

jona-sassenhagen · 2018-07-29T11:35:52Z

To be honest, I didn't sit down with the original paper for this one, I think I looked at a few implementations online and just made sure it matched the R output ... E.g.,

https://en.wikipedia.org/wiki/Talk:Varimax_rotation

jnothman · 2018-07-29T13:06:31Z

Thanks. This won't make it into 0.20 which we are trying to release. Please ping in a few weeks.

jona-sassenhagen · 2018-07-30T07:50:02Z

Ok, sure. Thanks.

jona-sassenhagen · 2018-10-01T12:14:44Z

Is now the time? :)

jnothman

Please provide references somewhere in the documentation (user guide and/or docstring)

jnothman · 2018-10-02T23:34:34Z

+
+        for _ in range(itermax):
+            comp_rot = np.dot(components, rotation_matrix)
+            tmp = np.diag((comp_rot ** 2).sum(axis=0)) / nrow * gamma


why do all this computation unnecessarily in the quartimax case?

Can't we just use if here?

jnothman · 2018-10-02T23:35:00Z

+    def _ortho_rotation(self, components, method='varimax', eps=1e-6,
+                        itermax=100):
+        """Return rotated components."""
+        if (method == 'varimax'):


parentheses should be removed here and below.

Can just use gamma = method == 'varimax', even.

jnothman · 2018-10-02T23:36:31Z

                          ConvergenceWarning)

        self.components_ = W
+        if self.rotation is not None:


might as well do this check inside the method

Actually, replace the previous line to self.components_ = self._rotate(W). I think that would be much clearer to the reader.

jnothman · 2018-10-02T23:36:45Z

+        "Rotate the factor analysis solution."
+        implemented = {"varimax", "quartimax"}
+        if method in implemented:
+            self.components_ = self._ortho_rotation(


It seems strange to put this in a separate function. Instead:

if method is None: return if method not in implemented: raise ... do the rotation self.components_ = ...

To make it simpler to test, you could make this a module-level private function or a staticmethod, taking components_ in and returning it optionally modified.

Ok sure. I'd for now keep it as a separate function to make it easier to refactor if somebody decides to add oblique or other rotations ..?

jnothman · 2018-10-02T23:37:06Z

+                np.dot(components.T, comp_rot ** 3 - np.dot(comp_rot, tmp)))
+            rotation_matrix = np.dot(u, v)
+            var_new = np.sum(s)
+            if var != 0 and (var_new < var * (1 + eps)):


parentheses around < expression are redundant

jnothman · 2018-10-02T23:37:22Z

                                  np.eye(X.shape[1]), 12)
+
+    # test rotation
+    n_comps = 2


call this n_components, please

jnothman · 2018-10-02T23:38:27Z

+            self.components_ = self._ortho_rotation(
+                self.components_.T, method=method, eps=tol)[:n_components]
+        else:
+            raise NotImplementedError("'method' must be in %s, not %s"


Usually we would use a ValueError. Subtle, yes, but NotImplementedError is usually for a function call with argument types where the operation is not yet supported, for instance.

jnothman · 2018-10-02T23:39:00Z

+    for method in (None, "varimax", 'quartimax'):
+        fa_var = FactorAnalysis(n_components=n_comps, rotation=method)
+        results.append(fa_var.fit_transform(X))
+    for a, b in ((0, 1), (0, 2), (1, 2)):


make the outer collection a list to improve readability [(0, 1), (0, 2) ...]

but it would be much more readable to use itertools.combinations([None, 'varimax', 'quartimax'], 2) and have stored the results by name

jnothman

Please fix up these changes and hopefully we can get another reviewer in towards merging this.

jnothman · 2020-04-07T02:07:01Z

+
+print(__doc__)
+
+# #############################################################################


Suggested change

# #############################################################################

##############################################################################

jnothman · 2020-04-07T02:07:08Z

+ax.set_title("Iris feature correlation matrix")
+plt.tight_layout()
+
+# #############################################################################


Suggested change

# #############################################################################

##############################################################################

Varimax PR: address review comments

jnothman

Thanks for fixing those up

jnothman · 2020-05-07T08:03:56Z

+
+print(__doc__)
+
+##############################################################################


We are now using the following for section dividers:

Suggested change

##############################################################################

# %%

ericpre · 2020-06-10T10:05:24Z

@amueller and @agramfort, are you happy with this PR?

agramfort

needs a versionadded added to docstring and update to what's new page. Besides LGTM !

agramfort · 2020-06-11T20:26:55Z

+        `"The varimax criterion for analytic rotation in factor analysis"
+        <https://link.springer.com/article/10.1007%2FBF02289233>`_
+        H. F. Kaiser, 1958
+


needs versionadded

Varimax

jnothman · 2020-07-05T12:26:17Z

@agramfort was that a formal approval?

agramfort

+1 for MRG provided CIs turn green. thx @jona-sassenhagen

ericpre · 2020-07-05T12:57:32Z

The CI failure seems to be related to setting up the environment, would updating the branch with upstream fix it? Last time, this branch was merged with upstream was 12 days ago.

thomasjpfan · 2020-07-05T15:23:55Z

The CI failure seems to be related to setting up the environment, would updating the branch with upstream fix it?

Yes merging with master should fix it. I have done to merge to see if it works.

jona-sassenhagen · 2020-07-05T16:42:35Z

We're green!

agramfort · 2020-07-05T19:29:18Z

Thx @jona-sassenhagen

jnothman · 2020-07-06T12:11:24Z

Thanks for your contribution, and for your patience, @jona-sassenhagen!

jona-sassenhagen · 2020-07-12T10:57:37Z

Thanks everyone.

@jnothman

* init varimax * fix py2 and add example text * fix print again * test, docs * fix test * fix doctstring * np.allclose for test * add refrence * address comments * update * Fix linting failure. * Fix deprecation warning. * a bit of cleanup * Address @jnothman's review comments. * Update section divider * Add note in what's new page and versionadded to docstring. Co-authored-by: Eric Prestat <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

@jnothman

* init varimax * fix py2 and add example text * fix print again * test, docs * fix test * fix doctstring * np.allclose for test * add refrence * address comments * update * Fix linting failure. * Fix deprecation warning. * a bit of cleanup * Address @jnothman's review comments. * Update section divider * Add note in what's new page and versionadded to docstring. Co-authored-by: Eric Prestat <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

init varimax

99991b7

jona-sassenhagen changed the title ~~WIP varimax rotation for factor analysis~~ [WIP] varimax rotation for factor analysis May 3, 2018

jona-sassenhagen commented May 3, 2018

View reviewed changes

Comment thread examples/decomposition/plot_varimax_fa.py

jona-sassenhagen added 2 commits May 3, 2018 16:46

fix py2 and add example text

b7e8e2c

fix print again

0a45384

jnothman reviewed May 6, 2018

View reviewed changes

jona-sassenhagen added 2 commits May 6, 2018 15:05

test, docs

9f582f0

fix test

a49506f

fix doctstring

68e6985

jnothman reviewed May 15, 2018

View reviewed changes

np.allclose for test

1171e0a

jona-sassenhagen changed the title ~~[WIP] varimax rotation for factor analysis~~ [MRG] varimax rotation for factor analysis Jun 2, 2018

jnothman reviewed Jun 7, 2018

View reviewed changes

jona-sassenhagen added 2 commits July 29, 2018 13:32

add refrence

c395f85

Merge branch 'master' into varimax

b7f0fd5

jona-sassenhagen mentioned this pull request Sep 26, 2018

Add Varimax rotation for Factor Analysis and PCA #2688

Closed

jnothman reviewed Oct 2, 2018

View reviewed changes

address comments

e930eb4

jnothman reviewed Apr 7, 2020

View reviewed changes

tjof2 mentioned this pull request Apr 29, 2020

Improve machine learning API and documentation hyperspy/hyperspy#2383

Merged

5 tasks done

ericpre added 2 commits April 29, 2020 15:31

Address @jnothman's review comments.

1eac444

Merge remote-tracking branch 'upstream/master' into varimax

d46be14

ericpre mentioned this pull request Apr 29, 2020

Varimax PR: address review comments jona-sassenhagen/scikit-learn#2

Merged

Merge pull request #2 from ericpre/varimax

b4b3bd0

Varimax PR: address review comments

jnothman approved these changes May 7, 2020

View reviewed changes

jnothman closed this May 7, 2020

jnothman reopened this May 7, 2020

Update section divider

10bc7e9

cmarmo added the Waiting for Reviewer label Jun 10, 2020

agramfort reviewed Jun 11, 2020

View reviewed changes

ericpre added 2 commits June 23, 2020 19:49

Merge remote-tracking branch 'upstream/master' into varimax

3fe1b64

Add note in what's new page and versionadded to docstring.

6dcc72e

ericpre mentioned this pull request Jun 23, 2020

Varimax jona-sassenhagen/scikit-learn#3

Merged

Merge pull request #3 from ericpre/varimax

6839797

Varimax

agramfort approved these changes Jul 5, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pr/11064

c860238

agramfort merged commit 5e193ec into scikit-learn:master Jul 5, 2020

jona-sassenhagen deleted the varimax branch July 12, 2020 10:57


		print(__doc__)

		# #############################################################################

	# #############################################################################
	##############################################################################

	##############################################################################
	# %%

Conversation

jona-sassenhagen commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

Uh oh!

sklearn-lgtm commented May 3, 2018

Uh oh!

jona-sassenhagen commented May 4, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jona-sassenhagen commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jona-sassenhagen commented May 12, 2018

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!

jona-sassenhagen commented May 30, 2018

Uh oh!

jona-sassenhagen commented Jun 2, 2018

Uh oh!

jona-sassenhagen commented Jun 7, 2018

Uh oh!

jnothman commented Jun 7, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jona-sassenhagen commented Jul 29, 2018

Uh oh!

jnothman commented Jul 29, 2018 via email

Uh oh!

jona-sassenhagen commented Jul 30, 2018

Uh oh!

jona-sassenhagen commented Oct 1, 2018

Uh oh!

jnothman left a comment

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!

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!

Uh oh!

Uh oh!

jnothman left a comment

jona-sassenhagen commented May 3, 2018 •

edited

Loading

jona-sassenhagen commented May 6, 2018 •

edited

Loading