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.

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

Jump to bottom

[ENH] Test and Score: Allow choosing columns #2257

Merged
merged 2 commits into from
Apr 28, 2017
Merged

[ENH] Test and Score: Allow choosing columns #2257

merged 2 commits into from
Apr 28, 2017

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Apr 21, 2017 •
edited
Loading

We may want to add more scorers in the future, but we'd like to keep the widget simple. This PR allows the user to choose the columns by right-clicking in the top header.

Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd force-pushed the test-learners-choose-columns branch from 788e862 to 87d7037 Compare April 21, 2017 14:04
@codecov-io
Copy link

codecov-io commented Apr 21, 2017 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@8e2f09b). Click here to learn what that means.
The diff coverage is 98.57%.

@@            Coverage Diff            @@
##             master    #2257   +/-   ##
=========================================
  Coverage          ?   72.51%           
=========================================
  Files             ?      319           
  Lines             ?    55198           
  Branches          ?        0           
=========================================
  Hits              ?    40025           
  Misses            ?    15173           
  Partials          ?        0

@ajdapretnar
Copy link
Contributor

I like this. It's a good solution to the 'kitchen sink' problem.
However, when running Test&Score, I get the following warning in the terminal:

/Users/ajda/orange/orange3/Orange/widgets/settings.py:378: UserWarning: Could not read defaults for widget <class 'Orange.widgets.evaluate.owtestlearners.OWTestLearners'>
The following error occurred:

'context_settings'
  .format(self.widget_class, ex))

It is not related to this PR, I get it on the latest master as well, but it seems to be happening in Test&Score and perhaps it can be patched here. Or in another PR... Oh, and it doesn't break anything obvious.

@janezd
Copy link
Contributor Author

janezd commented Apr 24, 2017

I've been told by @astaric that the aforementioned error has already been patched by @lanzagar in another PR.

@janezd janezd force-pushed the test-learners-choose-columns branch from 8221cc7 to b2f85a8 Compare April 24, 2017 09:59
astaric
astaric approved these changes Apr 28, 2017
View reviewed changes
Copy link
Member

@astaric astaric left a comment

Choose a reason for hiding this comment

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

Looks good to me

Orange/widgets/evaluate/owtestlearners.py Outdated
def show_column_chooser(self, pos):
# pylint doesn't know that self.shown_scores is a set, not a Setting
# pylint: disable=unsupported-membership-test
def update(col_name, state):
Copy link
Member

Choose a reason for hiding this comment

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

state -> visible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps misleading since it would imply it is (was) already visible. I changed it to checked.

Orange/widgets/evaluate/tests/test_owtestlearners.py
return action

def execmenu(*_):
self.assertEqual(list(actions), list(all)[1:])
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining why complication with execmenu is needed would be nice

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.

Orange/widgets/evaluate/owtestlearners.py
@@ -170,6 +170,10 @@ class OWTestLearners(OWWidget):
TARGET_AVERAGE = "(Average over classes)"
class_selection = settings.ContextSetting(TARGET_AVERAGE)

shown_scores = \
settings.Setting({"AUC", "CA", "F1", "Precision", "Recall",
Copy link
Member

Choose a reason for hiding this comment

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

Non-primitive settings cannot be stored to workflow in json format resulting in base64 encoded pickled blobs. Not sure if we care, since the widget uses context settings, which also cannot be serialized with json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If list can be stored in json and if we care(d) I can replace the set with a list. No big changes in the code, except that conceptually this is a set, not a list.

Can it, do we, should I?

Copy link
Member

Choose a reason for hiding this comment

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

yes, probably not, no

If this change would "break" json serialization, I would say yes, but since it is already broken, I would say leave it like it is.

@janezd janezd force-pushed the test-learners-choose-columns branch from b2f85a8 to 3212e3a Compare April 28, 2017 07:54
@astaric astaric merged commit dbab861 into biolab:master Apr 28, 2017
@janezd janezd mentioned this pull request Apr 28, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astaric astaric astaric approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@janezd @codecov-io @ajdapretnar @astaric