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] Distances: handling errors due to too large arrays #2315

Merged
merged 2 commits into from
Jun 9, 2017
Merged

[FIX] Distances: handling errors due to too large arrays #2315

merged 2 commits into from
Jun 9, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented May 15, 2017

Issue

https://sentry.io/biolab/orange3/issues/196744070/events/5529227746/

Description of changes

Users sees an error message when calculating too large arrays and Orange does not crash.

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented May 15, 2017

@codecov-io
Copy link

codecov-io commented May 15, 2017

Codecov Report

Merging #2315 into master will increase coverage by 0.01%.
The diff coverage is 79.54%.

@@            Coverage Diff             @@
##           master    #2315      +/-   ##
==========================================
+ Coverage   73.27%   73.29%   +0.01%     
==========================================
  Files         317      317              
  Lines       55447    55469      +22     
==========================================
+ Hits        40630    40654      +24     
+ Misses      14817    14815       -2

@janezd
Copy link
Contributor

janezd commented May 20, 2017

@astaric, similar code warnings are stupid. See this -- codeclimate complained about two totally different lists of error in class Error. I suppose it will complain about all such classes?

@@ -48,6 +48,8 @@ class Error(OWWidget.Error):
dense_metric_sparse_data = Msg("Selected metric does not support sparse data")
empty_data = Msg("Empty data set")
mahalanobis_error = Msg("{}")
distances_memory_error = Msg("Not enough memory.")
distances_value_error = Msg("Error occurred while calculating distances: {}")
Copy link
Contributor

Choose a reason for hiding this comment

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

The error can be long, so "Error occurred while calculating distances\n{}". Skip the colon, so the error looks OK, and the tooltip shows the entire message.

@@ -144,7 +157,7 @@ def compute_distances(self, metric, data):
self.Error.mahalanobis_error(e)
return

return metric(data, data, 1 - self.axis, impute=True)
return compute(metric, data)
Copy link
Contributor

Choose a reason for hiding this comment

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

A stylistic comment. I don't like the obvious radon fixes --- here's a code that's too complex so let's tear out a chunk of it into a separate function.

The main job of compute_distances is not to run various checks and fixes, and in the end, yes, call a local function that computes distances.

Turn it around. The "auxiliary" code should be the code that prepares the data and not the code that does the work. So, factor out the preprocessing/checking instead.


mock = Mock()
mock.side_effect = ValueError
self.widget.compute_distances(mock, self.iris)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use side_effect as a keyword argument for mock. I wouldn't even bother to have a mock variable here; I'd use Mock(side_effect=ValueError) as an argument.


mock = Mock()
mock.side_effect = MemoryError
self.widget.compute_distances(mock, self.iris)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@jerneju
Copy link
Contributor Author

jerneju commented May 23, 2017

Fixed.

@jerneju
Copy link
Contributor Author

jerneju commented May 29, 2017

@jerneju jerneju added this to the 3.4.3 milestone Jun 2, 2017
@nikicc nikicc modified the milestones: future, 3.4.3 Jun 3, 2017
@janezd janezd self-assigned this Jun 9, 2017
@janezd janezd merged commit 8236534 into biolab:master Jun 9, 2017
@jerneju jerneju deleted the value-distances branch June 9, 2017 08:15
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.

4 participants