Skip to content
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.

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

[FIX] Manifold Learning: handling numpy LinAlgError #2228

Merged
merged 1 commit into from
Apr 21, 2017
Merged

[FIX] Manifold Learning: handling numpy LinAlgError #2228

merged 1 commit into from
Apr 21, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Apr 12, 2017

Issue

https://sentry.io/biolab/orange3/issues/210202559/

Description of changes

Sometimes singular matrix is calculated from input data and that is now handled.

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Apr 12, 2017

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #2228 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2228      +/-   ##
==========================================
+ Coverage   72.26%   72.28%   +0.01%     
==========================================
  Files         318      318              
  Lines       54856    54867      +11     
==========================================
+ Hits        39644    39658      +14     
+ Misses      15212    15209       -3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d61610...f427367. Read the comment docs.

@@ -260,6 +262,8 @@ def apply(self):
self.Error.n_neighbors_too_small("{}".format(n))
else:
self.Error.manifold_error(e.args[0])
except np.linalg.linalg.LinAlgError as e:
self.Error.manifold_error(e.args[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Use str(e) instead of e.args[0].
Calling str() on an instance of BaseException will give something at least as good as `e.args[0].

Actually, even self.Error.manifold_error(e) but I'd prefer being explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

def test_data_which_makes_singular(self):
"""
Sometimes singular matrix is calculated from input data and
that should be handled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rephrase: Handle singular matrices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -73,3 +73,25 @@ def test_sparse_data(self):
self.send_signal("Data", None)
self.widget.apply_button.button.click()
self.assertFalse(self.widget.Error.sparse_not_supported.is_shown())

def test_data_which_makes_singular(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

test_singular_matrices or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

list(zip(
[1, 1, 1],
[0, 1, 2],
[0, 1, 1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a simpler, more obviously singular matrix, like all zeros, work here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, matrix with all zeros would work here as well. But I just wanted to point out that regular matrix can crash the code as well.

@janezd janezd merged commit 7ae2cfb into biolab:master Apr 21, 2017
@jerneju jerneju deleted the linalg-manifoldlearning branch April 21, 2017 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants