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] Sieve Diagram: Using datasets with meta data #2098

Merged
merged 2 commits into from
Mar 18, 2017
Merged

[FIX] Sieve Diagram: Using datasets with meta data #2098

merged 2 commits into from
Mar 18, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Mar 13, 2017

Issue

Widget should intepret meta data which are continuous or discrete in the same way
as features or target.
https://sentry.io/biolab/orange3/issues/231234770/

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Mar 14, 2017

@codecov-io
Copy link

codecov-io commented Mar 16, 2017

Codecov Report

Merging #2098 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2098      +/-   ##
==========================================
- Coverage   71.48%   71.31%   -0.17%     
==========================================
  Files         318      318              
  Lines       54393    54278     -115     
==========================================
- Hits        38881    38709     -172     
- Misses      15512    15569      +57

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 4e812a9...7656c07. Read the comment docs.

@@ -167,13 +167,10 @@ def set_data(self, data):
self.domain_model.set_domain(None)
else:
self.domain_model.set_domain(data.domain)
if any(attr.is_continuous for attr in data.domain):
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here was to avoid unnecessary calls to Discretize. Wouldn't it be better to just replace the above condition with something like

if any(attr.is_continuous for attr in chain(data.domain, data.domain.metas)):

and leave the rest as it is?

[42.48, 16.84, 15.23, 23.8],
"yynn"))
)
self.send_signal("Data", table)
Copy link
Contributor

Choose a reason for hiding this comment

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

This checks just that the widget does not crash. It is difficult to test this widget assuming anything about the widget internals. For the lack of better options, I'd replace self.send_signal("Data", table) with

        from unittest.mock import patch
        from Orange.widgets.visualize.owsieve import Discretize
        with patch("Orange.widgets.visualize.owsieve.Discretize",
                   wraps=Discretize) as disc:
            self.send_signal("Data", table)
            self.assertTrue(disc.called)
        metas = self.widget.discrete_data.domain.metas
        self.assertEqual(len(metas), 2)
        self.assertTrue(all(attr.is_discrete for attr in metas))

Move imports to the beginning of the file, of course.

Issue
Widget should intepret meta data which are continuous or discrete in the same way
as features or target.
https://sentry.io/biolab/orange3/issues/231234770/
https://sentry.io/biolab/orange3/issues/232101010/
@janezd janezd merged commit 2d4aa5a into biolab:master Mar 18, 2017
@jerneju jerneju deleted the typeerror-owsieve branch April 20, 2017 13:47
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