Skip to content

[FIX] OWBoxPlot: Fixups#1783

Merged
lanzagar merged 2 commits intobiolab:masterfrom
VesnaT:fix_bp
Dec 1, 2016
Merged

[FIX] OWBoxPlot: Fixups#1783
lanzagar merged 2 commits intobiolab:masterfrom
VesnaT:fix_bp

Conversation

@VesnaT
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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