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] Correspondence Analysis: do not crash when no categorical #2723

Merged
merged 2 commits into from
Oct 31, 2017
Merged

[FIX] Correspondence Analysis: do not crash when no categorical #2723

merged 2 commits into from
Oct 31, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Oct 30, 2017

Issue

Widget crashes when there are no categorical variables in a data.

Description of changes

Check and show error.

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Oct 30, 2017

@codecov-io
Copy link

codecov-io commented Oct 30, 2017

Codecov Report

Merging #2723 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2723      +/-   ##
==========================================
+ Coverage   76.03%   76.04%   +<.01%     
==========================================
  Files         338      338              
  Lines       59628    59643      +15     
==========================================
+ Hits        45339    45356      +17     
+ Misses      14289    14287       -2

janezd
janezd previously requested changes Oct 31, 2017
Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

Some minor changes, otherwise OK.

@@ -61,6 +61,7 @@ class Inputs:

class Error(widget.OWWidget.Error):
empty_data = widget.Msg("Empty data set")
no_disc_vars = widget.Msg("Correspondence Analysis needs categorical variables")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a period at the end (we follow the rule that if the message is a complete sentence, we add a period).

Alternatively, shorten this to "No categorical variables" (without a period).

self._restore_selection()
# self._invalidate()
if not len(self.varlist[:]):
self.Error.no_disc_vars()
Copy link
Contributor

Choose a reason for hiding this comment

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

Add self.data = None here. Not needed right now, but the above code sets it to None as the indicator that no data is shown.

self.send_signal(self.widget.Inputs.data, table)
self.assertTrue(self.widget.Error.no_disc_vars.is_shown())
self.send_signal(self.widget.Inputs.data, None)
self.assertFalse(self.widget.Error.no_disc_vars.is_shown())
Copy link
Contributor

Choose a reason for hiding this comment

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

Add

        self.send_signal(self.widget.Inputs.data, table)
        self.assertTrue(self.widget.Error.no_disc_vars.is_shown())
        self.send_signal(self.widget.Inputs.data, Table("iris"))
        self.assertFalse(self.widget.Error.no_disc_vars.is_shown())

We have had bugs in the past where the error message was sometimes not cleared.

@@ -102,6 +103,7 @@ def __init__(self):
def set_data(self, data):
self.closeContext()
self.clear()
self.Error.no_disc_vars.clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the widget is reset here, I suppose that no error should remain visible from the previous data. So you can safely call self.Error.clear() instead of self.Error.no_disc_vars.clear(), and then you can also remove else: self.Error.empty_data.clear() below.

@janezd
Copy link
Contributor

janezd commented Oct 31, 2017

Actually, I can take care of my nitpicking myself. :)

@janezd janezd merged commit 9225e8d into biolab:master Oct 31, 2017
@jerneju jerneju deleted the fix-correspondenceanalysis-no-categorical branch November 2, 2017 06:46
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