[ENH] Improved Sparsity Handling by nikicc · Pull Request #2341 · biolab/orange3

nikicc · 2017-05-25T10:53:45Z

Issue

Fixes #2322.

Blocked on #2734. Blocked on #2736.

Description of changes

DomainConversion now has three extra attributes telling whether X, Y, or metas should be sparse according to the features inside of them. Metas can only be sparse if no StringVariables are present.
Table.from_table determines the density of X, Y, and metas according to domain conversion.
Add to_sparse and to_dense methods to Table.
Reset variables caches to alleviate interdependencies between test files.
Fix showing sparse metas in Table widget.

Includes

Code changes
Tests
Documentation

jerneju · 2017-05-31T10:08:28Z

That PR should fix #2359 .

jerneju

When someone wants to transform two columns from sparse X to dense Y she or he gets only one Y column which has got double the number of rows.

jerneju · 2017-05-31T10:19:25Z

table = Table("iris")
table.X = sp.csr_matrix(table.X)
attributes = table.domain.attributes[:2] + table.domain.class_vars
class_vars = table.domain.attributes[2:4]
newtable = table.transform(Domain(attributes=attributes, class_vars=class_vars))
self.assertEqual(newtable.Y.shape[0], table.Y.shape[0])

nikicc · 2017-06-01T14:44:02Z

@jerneju nice catch. Should be fixed not, please check.

jerneju · 2017-06-01T14:56:06Z

Orange/tests/test_domain.py


        self.assertLessEqual(time() - start, 1)

+    def test_domain_conversion_sparsity(self):


I usually add something like this:

""" Description. GH-2341 """

lanzagar · 2017-06-02T13:01:24Z

Orange/data/variable.py

+    .. attribute:: sparse
+
+        A flag about sparsity of the variable. When set, the variable suggests
+        it is should be stored in a sparse matrix. 


codecov-io · 2017-06-02T22:13:44Z

Codecov Report

Merging #2341 into master will increase coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff            @@
##           master   #2341      +/-   ##
=========================================
+ Coverage   76.09%   76.1%   +0.01%     
=========================================
  Files         338     338              
  Lines       59723   59760      +37     
=========================================
+ Hits        45444   45479      +35     
- Misses      14279   14281       +2

astaric · 2017-11-10T09:54:44Z

Orange/data/domain.py

            source.index(var) if var in source
            else var.compute_value for var in destination.metas]

+        def is_sparse(feats):


should_be_sparse?

That shoulds better 👍 Fixed.

astaric · 2017-11-10T10:03:15Z

Orange/data/table.py


-        def get_columns(row_indices, src_cols, n_rows, dtype=np.float64,
-                        is_sparse=False):
+        def array_transform(x, to_sparse):


Could these functions be moved someplace else? They seem like general functions that ensure array/column is sparse/dense. Maybe change to a more expressive name.

lanzagar · 2017-11-10T11:08:37Z

Orange/data/domain.py

+        def is_sparse(feats):
+            """ For a matrix to be stored in sparse, more than 2/3 of columns
+            should be marked as sparse and there should be no string columns
+            since Scipy's sparse matrices don't support dtype=object."""


At least the closing triple quotes of a multi-line docstring should be in their own line (PEP257).
(for opening quotes try to mimic other docstrings nearby)

Changed to match the style as in nerby docstrings.

lanzagar · 2017-11-10T11:08:57Z

Orange/data/domain.py

+            """ For a matrix to be stored in sparse, more than 2/3 of columns
+            should be marked as sparse and there should be no string columns
+            since Scipy's sparse matrices don't support dtype=object."""
+            fraction_sparse = sum(f.sparse for f in feats)/max(len(feats), 1)


space around operators

nikicc · 2017-11-10T15:13:30Z

@astaric, @markotoplak I split the functionality of array_transform and column_transform into 4 helper functions (see the last two fixup commints 4410596, 3d1a96d). Do you think that's better?

I haven't moved the functions yet, though. I will do this once we settle on the best approach. Perhaps to some util library?

DomainConversion now has three more attributes `sparse_X`, `sparse_Y` and `sparse_metas`, which suggest whether the resulting matrix should be sparse or dense.

Table.from_table now sets the sparsity of X, Y and metas as determined by DomainConversion.

When we return subarryas, the flag `is_sparse` wasn't considered, but we simpy returned the subarray in it's original format. Also, make sure subarrays aren't flattened to 1d, as it is required for columns.

Calling len on scipy sparse matrices causes the error: `TypeError: sparse matrix length is ambiguous; use getnnz() or shape[0]`.

... to alleviate interdependencies between tests in differente files. Before that, one tests could mark some attributes of iris as sparse which could cause tests in different files to crash due to variable "reusing".

nikicc added the DH2017 label May 25, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 3 times, most recently from 4b65c4e to 80475e7 Compare May 25, 2017 11:29

jerneju mentioned this pull request May 31, 2017

Table: when Y has only one column it is always dense #2361

Closed

3 tasks

jerneju suggested changes May 31, 2017

View reviewed changes

nikicc force-pushed the sparsity-from-domain-conversion branch from 125a142 to 71fc394 Compare June 1, 2017 14:37

jerneju approved these changes Jun 1, 2017

View reviewed changes

nikicc added this to the 3.4.3 milestone Jun 2, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 7 times, most recently from 942f527 to 120f92e Compare June 2, 2017 12:37

lanzagar reviewed Jun 2, 2017

View reviewed changes

nikicc force-pushed the sparsity-from-domain-conversion branch 6 times, most recently from 4d20017 to c23bf8d Compare June 2, 2017 22:13

nikicc force-pushed the sparsity-from-domain-conversion branch 2 times, most recently from 8488448 to 68594d1 Compare June 2, 2017 22:58

nikicc removed this from the 3.4.3 milestone Jun 3, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 3 times, most recently from 3daedd2 to 0584b67 Compare November 3, 2017 10:19

nikicc added this to the 3.8 milestone Nov 3, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 8 times, most recently from ac4ae07 to 34f78a7 Compare November 3, 2017 14:14

astaric reviewed Nov 10, 2017

View reviewed changes

lanzagar reviewed Nov 10, 2017

View reviewed changes

nikicc added 14 commits November 17, 2017 10:39

DomainConversion: Add sparsity suggestions

951fd8e

DomainConversion now has three more attributes `sparse_X`, `sparse_Y` and `sparse_metas`, which suggest whether the resulting matrix should be sparse or dense.

Table: Set density according to DomainConversion

7fcdbde

Table.from_table now sets the sparsity of X, Y and metas as determined by DomainConversion.

Data.utils: Add methods for assuring sparse/dense

bef9c4b

Table.from_table: Obey is_sparse when returning subarrays

7fa9f05

When we return subarryas, the flag `is_sparse` wasn't considered, but we simpy returned the subarray in it's original format. Also, make sure subarrays aren't flattened to 1d, as it is required for columns.

Table: Consider sparsity flags when domain match

6f004ea

Table: Add to_sparse and to_dense methods

332af4e

Table._check_arrays: Don't use len on sparse matrices

204c2d4

Calling len on scipy sparse matrices causes the error: `TypeError: sparse matrix length is ambiguous; use getnnz() or shape[0]`.

TableModel: Fix showing sparse metas

228afd4

Tests: Fixups according to improved sparsity

e57f340

TestTable: Use transform instead of trom_table

a9bd8ca

WidgetTest, WidgetOutputsTestMixin: Reset variables' cache

a0935fa

... to alleviate interdependencies between tests in differente files. Before that, one tests could mark some attributes of iris as sparse which could cause tests in different files to crash due to variable "reusing".

Gui: Fixups according to improved sparsity

8ed204d

Add tests Table.from_table sparsity hadling

7af1181

PyLint

4bf6a20


		self.assertLessEqual(time() - start, 1)

		def test_domain_conversion_sparsity(self):

Uh oh!

Conversation

nikicc commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description of changes

Includes

Uh oh!

jerneju commented May 31, 2017

Uh oh!

jerneju left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

nikicc commented Jun 1, 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!

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

Uh oh!

Codecov Report

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!

nikicc commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nikicc commented May 25, 2017 •

edited

Loading

jerneju commented May 31, 2017 •

edited

Loading

codecov-io commented Jun 2, 2017 •

edited

Loading

nikicc commented Nov 10, 2017 •

edited

Loading