-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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] OWBoxPlot: Fixups #1783
[FIX] OWBoxPlot: Fixups #1783
Conversation
Current coverage is 88.96% (diff: 100%)@@ master #1783 diff @@
==========================================
Files 82 82
Lines 8963 8963
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 7974 7974
Misses 989 989
Partials 0 0
|
""" | ||
if not include_class: | ||
return any(var.is_discrete for var in self.attributes) | ||
attributes = list(self.attributes) if not include_metas \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) Isn't the conversion to lists unnecessary?
b) This whole function can be shortened even further, e.g.:
vars = self.variables if include_class else self.attributes
vars += self.metas if include_metas else ()
return any(var.is_discrete for var in vars)
""" | ||
if not include_class: | ||
return any(var.is_continuous for var in self.attributes) | ||
attributes = list(self.attributes) if not include_metas \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for discrete above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another small fix for Box Plot that can be added in this PR perhaps:
After moving features to meta, if you remove the class too, nothing is shown (should show metas). This is because line 259 checks the domain len (which does not include metas).
Can you change it to len(dataset.domain.variables + dataset.domain.metas)
or something like that (can't remember a better/shorter way now).
EDIT: Hm, I guess only primitive metas should count since others are ignored.
This should be done in another pull request, since it requires some changes with Context settings. |
Issue
The widget crashed if continuous variables were only in metas.
Description of changes
To reproduce, use File (iris) -> Select columns -> Feature constructor -> Box plot, to move continuous features to metas and to create a new StringVariable, which causes data.metas.dtype = object.
Includes