Skip to content

[FIX] Table: Fix ensure_copy for sparse matrices #1456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
astaric merged 1 commit into from
Jul 15, 2016
Merged

[FIX] Table: Fix ensure_copy for sparse matrices #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions Orange/data/table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,15 +912,20 @@ def is_copy(self):

def ensure_copy(self):
"""
Ensure that the table owns its data; copy arrays when necessary
Ensure that the table owns its data; copy arrays when necessary.
"""
if self.X.base is not None:
def is_view(x):
# Sparse matrices don't have views like numpy arrays. Since indexing on
# them creates copies in constructor we can skip this check here.
return not sp.issparse(x) and x.base is not None

if is_view(self.X):
self.X = self.X.copy()
Copy link
Copy Markdown
Contributor

@kernc kernc Jul 14, 2016

Choose a reason for hiding this comment

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

But if they are sparse, no copy is made?

Copy link
Copy Markdown
Contributor Author

@nikicc nikicc Jul 14, 2016

Choose a reason for hiding this comment

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

Currently not, this is left to the constructor. As far as I understand ensure_copy only ensures that the data is not a view and makes a copy if it is. Since sparse matrices do not have views (at least I think so) I propose to skip the check. Currently the copy is made in the constructor anyway and hence we probably do not need to make an other here.

if self._Y.base is not None:
if is_view(self._Y):
self._Y = self._Y.copy()
if self.metas.base is not None:
if is_view(self.metas):
self.metas = self.metas.copy()
if self.W.base is not None:
if is_view(self.W):
self.W = self.W.copy()

def copy(self):
Expand Down
13 changes: 13 additions & 0 deletions Orange/tests/test_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,19 @@ def test_copy(self):
self.assertFalse(np.all(t.Y == copy.Y))
self.assertFalse(np.all(t.metas == copy.metas))

def test_copy_sparse(self):
t = data.Table('iris')
t.X = csr_matrix(t.X)
copy = t.copy()

self.assertEqual((t.X != copy.X).nnz, 0) # sparse matrices match by content
np.testing.assert_equal(t.Y, copy.Y)
np.testing.assert_equal(t.metas, copy.metas)

self.assertNotEqual(id(t.X), id(copy.X))
self.assertNotEqual(id(t._Y), id(copy._Y))
self.assertNotEqual(id(t.metas), id(copy.metas))

def test_concatenate(self):
d1 = data.Domain([data.ContinuousVariable('a1')])
t1 = data.Table.from_numpy(d1, [[1],
Expand Down