[ENH] FreeViz script by jerneju · Pull Request #2563 · biolab/orange3

jerneju · 2017-09-07T12:00:17Z

Issue

No FreeViz projection.

Description of changes

FreeViz projection scripting part similar as MDS, PCA.

from Orange.projection.freeviz import FreeViz
from Orange.data import Table
data = Table("iris")
freeviz = FreeViz()
model = freeviz(data)
new_data = model(data)

Code coverage is 98%.

Includes

Code changes
Tests
Documentation

codecov-io · 2017-09-07T13:20:28Z

Codecov Report

Merging #2563 into master will increase coverage by 0.07%.
The diff coverage is 93.85%.

@@            Coverage Diff             @@
##           master    #2563      +/-   ##
==========================================
+ Coverage   75.65%   75.73%   +0.07%     
==========================================
  Files         333      334       +1     
  Lines       58422    58654     +232     
==========================================
+ Hits        44202    44422     +220     
- Misses      14220    14232      +12

jerneju · 2017-09-14T11:51:24Z

@janezd

janezd · 2017-09-14T17:46:01Z

Could you rebase #2512 to this, so I can observe this PR in actual action? You'll have to do it some day anyway.

#2512 does not have to be finished, I just want to see how the projection function works.

jerneju · 2017-09-19T14:52:06Z

@janezd: Rebased.

jerneju · 2017-10-09T07:21:37Z

@janezd : Is this ok to merge?

janezd · 2017-10-09T16:42:50Z

Orange/projection/freeviz.py

+            X = data.X
+            Y = data.Y
+        if X is None or Y is None:
+            return


return None, None, None

janezd · 2017-10-09T17:08:45Z

Orange/projection/freeviz.py

+    def __init__(self, weights=None, center=True, scale=True, dim=2, p=1,
+                 initial=None, maxiter=500, alpha=0.1, atol=1e-5, preprocessors=None):
+        super().__init__(preprocessors=preprocessors)
+        self.params = vars()


We use this monstrosity only in SKL wrappers. I think FreeViz does not need params.

janezd · 2017-10-09T17:12:56Z

Orange/projection/freeviz.py

+
+    def __call__(self, data):
+        if data and data.domain.class_var.is_discrete:
+            self.is_class_discrete = True


The same instance of FreeViz can be called multiple times and with different data sets, hence it can't count that is_class_discrete is False (as set in the constructor`.

if data is not None: self.is_class_discrete = data.domain.class_var.is_discrete

janezd · 2017-10-09T17:15:22Z

Orange/projection/freeviz.py

+    def transform(self, X):
+        EX = np.dot(X, self.components_)
+        return EX
+


@markotoplak, the code from the beginning of the file and up to here is adapted from PCA. I know the idea, but you're an expert here. Can you take a quick look, please?

Seems OK.

I think it would be useful to add a test, where we would

open data, apply some preprocessor, splits the data into two parts, use FreeViz on the first part, and then transform the second part,

open data, split into two parts, apply the same preprocessor and FreeViz only on the first part, and then transform the second part,

where the transformed second part in (1) and (2) has to be the same.

janezd · 2017-10-09T17:18:25Z

Orange/projection/freeviz.py

+        return self.freeviz.transform(data.X)
+
+
+class _LinearCombination:


@lanzagar, could Orange.projection.pca._LinearCombination be moved to Orange.projection.base.LinearCombination`, so it could be reused here?

Also, PCAModel is just a specific case of linear model. Could the common functionality be extracted into a base class? Could you and @jerneju discuss this? I don't insist it should be done, I'm just suggesting considering it. And it doesn't have to be in this PR, either.

Could and should.

Could. Probably should? Definitely should be considered.

We'll take a look.

markotoplak · 2017-10-10T08:08:51Z

Orange/projection/freeviz.py

+        self.transformed = None
+
+    def compute(self, data, freeviz_space):
+        if self.feature >= freeviz_space.shape[1]:


How could this happen at all?

Thanks. Fixed.

markotoplak · 2017-10-10T08:29:59Z

Orange/projection/freeviz.py

+    def transform(self, X):
+        EX = np.dot(X, self.components_)
+        return EX
+


Seems OK.

I think it would be useful to add a test, where we would

open data, apply some preprocessor, splits the data into two parts, use FreeViz on the first part, and then transform the second part,

open data, split into two parts, apply the same preprocessor and FreeViz only on the first part, and then transform the second part,

where the transformed second part in (1) and (2) has to be the same.

lanzagar · 2017-10-10T10:24:42Z

Orange/projection/freeviz.py

+from Orange.data.util import SharedComputeValue
+from Orange.projection import Projector, Projection
+
+


Define __all__ here and pull FreeViz up into Orange.projection.
Tests and documentation should then import from there.

janezd

@markotoplak, can you check and approve the changes?

@jerneju, can you commit --amend & push -f again to restart travis?

janezd · 2017-10-12T08:16:50Z

Orange/projection/base.py

+__all__ = ["LinearCombinationSql", "Projector", "Projection", "SklProjector"]
+
+
+class LinearCombinationSql:


I don't know what this actually does, but I suppose that this is a general linear combination, and it also supports SQL -- and not a linear combination that requires SQL, right? If so, the name is misleading. @lanzagar, could you help here?

This is the sql equivalent of the compute_value. It only works for sql, as in-memory data is processed with a different object - Projector.

Ah, OK, I see. Thanks.

janezd · 2017-10-13T08:16:26Z

Orange/tests/test_freeviz.py

+
+    def test_transform_changed_domain(self):
+        """
+        Thest where:


Thest -> test

janezd · 2017-10-13T08:20:10Z

Orange/tests/test_freeviz.py

+        model = freeviz(ndata)
+        result_2 = model(data[100:])
+
+        self.assertTrue(np.allclose(result_1, result_2))


Why not np.testing.assert_almost_equal?

@markotoplak, the test is otherwise OK, right?

======================================================================
ERROR: test_transform_changed_domain (Orange.tests.test_freeviz.TestFreeviz)

Traceback (most recent call last):
File "/home/travis/build/biolab/orange3/build/travis-test/Orange/tests/test_freeviz.py", line 101, in test_transform_changed_domain
np.testing.assert_almost_equal(result_1, result_2)
File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/numpy/testing/utils.py", line 568, in assert_almost_equal
if not (gisfinite(desired) and gisfinite(actual)):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Maybe

numpy.testing.assert_allclose

The test is almost OK.

To make results repeatable, init_random should be fixed so that it uses rstate = np.random.RandomState(0) if not given. Then, inititalization should be removed from the test.

The comment on the test is superfluous - the code and two in code comments say it all.

jerneju changed the title ~~[ENH] Freeviz script~~ [ENH] FreeViz script Sep 7, 2017

jerneju added 3 commits September 19, 2017 16:47

Freeviz script: copy from Prototypes

4f6f205

Freeviz script: numpy -> np

059898e

Freeviz script: class version

daeb537

jerneju added this to the 3.6 milestone Sep 29, 2017

astaric removed this from the 3.6 milestone Sep 29, 2017

jerneju mentioned this pull request Oct 6, 2017

[ENH] FreeViz: new widget #2512

Merged

3 tasks

janezd reviewed Oct 9, 2017

View reviewed changes

markotoplak requested changes Oct 10, 2017

View reviewed changes

lanzagar reviewed Oct 10, 2017

View reviewed changes

PCA: class _LinearCombination moved to base

6896d1b

janezd reviewed Oct 12, 2017

View reviewed changes

janezd reviewed Oct 13, 2017

View reviewed changes

Orange/tests/test_freeviz.py Outdated

def test_transform_changed_domain(self):

"""

Thest where:

Copy link
Copy Markdown

Contributor

janezd Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thest -> test

janezd reviewed Oct 13, 2017

View reviewed changes

jerneju added 3 commits October 13, 2017 11:56

Freeviz script: work similar as PCA

b4fc372

Freeviz script: add tests

5c5efd9

Freeviz script: documentation

0352057

markotoplak approved these changes Oct 13, 2017

View reviewed changes

FreeViz Script: add preprocessor and transform test

3769d62

janezd merged commit 6e04e28 into biolab:master Oct 13, 2017

		return self.freeviz.transform(data.X)


		class _LinearCombination:

		from Orange.data.util import SharedComputeValue
		from Orange.projection import Projector, Projection

		__all__ = ["LinearCombinationSql", "Projector", "Projection", "SklProjector"]


		class LinearCombinationSql:

Uh oh!

Conversation

jerneju commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description of changes

Includes

Uh oh!

codecov-io commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jerneju commented Sep 14, 2017

Uh oh!

janezd commented Sep 14, 2017

Uh oh!

jerneju commented Sep 19, 2017

Uh oh!

jerneju commented Oct 9, 2017

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!

janezd 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

====================================================================== ERROR: test_transform_changed_domain (Orange.tests.test_freeviz.TestFreeviz)

Uh oh!

jerneju Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jerneju commented Sep 7, 2017 •

edited

Loading

codecov-io commented Sep 7, 2017 •

edited

Loading

======================================================================
ERROR: test_transform_changed_domain (Orange.tests.test_freeviz.TestFreeviz)

jerneju Oct 13, 2017 •

edited

Loading