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] OWBoxPlot: Fixups #1783

Merged
merged 2 commits into from
Dec 1, 2016
Merged

[FIX] OWBoxPlot: Fixups #1783

merged 2 commits into from
Dec 1, 2016

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Nov 29, 2016

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
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Nov 29, 2016

Current coverage is 88.96% (diff: 100%)

Merging #1783 into master will not change coverage

@@             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          

Sunburst

Powered by Codecov. Last update 4975a33...920ced4

@VesnaT VesnaT changed the title OWBoxPlot: Fix crash when continuous variables appear only in metas OWBoxPlot: Fixups Nov 30, 2016
@VesnaT VesnaT changed the title OWBoxPlot: Fixups [FIX] OWBoxPlot: Fixups Nov 30, 2016
@VesnaT VesnaT added this to the 3.3.9 milestone Nov 30, 2016
"""
if not include_class:
return any(var.is_discrete for var in self.attributes)
attributes = list(self.attributes) if not include_metas \
Copy link
Contributor

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 \
Copy link
Contributor

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.

Copy link
Contributor

@lanzagar lanzagar left a 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.

@lanzagar lanzagar self-assigned this Dec 1, 2016
@VesnaT
Copy link
Contributor Author

VesnaT commented Dec 1, 2016

This should be done in another pull request, since it requires some changes with Context settings.

@lanzagar lanzagar merged commit e069705 into biolab:master Dec 1, 2016
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