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

[ENH] OWBoxPlot: add option to sort discrete distributions by size #3156

Merged
merged 4 commits into from
Aug 24, 2018

Conversation

matejklemen
Copy link
Contributor

Issue

Implements #3093.

Description of changes

Added option (checkbox) to allow sorting discrete distributions by their size. If checkbox is unchecked, original order is preserved.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Jul 25, 2018

Codecov Report

Merging #3156 into master will increase coverage by 0.27%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #3156      +/-   ##
==========================================
+ Coverage   82.36%   82.63%   +0.27%     
==========================================
  Files         336      342       +6     
  Lines       58289    58995     +706     
==========================================
+ Hits        48011    48752     +741     
+ Misses      10278    10243      -35

@lanzagar lanzagar added this to the 3.16 milestone Aug 3, 2018
@lanzagar
Copy link
Contributor

lanzagar commented Aug 3, 2018

Looks good, but sometimes while toggling the sorting the figure is not redrawn properly.
I tried it on cyber-security-breaches and changed Subgroups between None and US State, scrolled to the top of the visualization and then enabled/disabled sorting. Scrolling down and up forces the redraw and it again looks ok.
Needs some investigation.

boxplot1

boxplot2

@matejklemen
Copy link
Contributor Author

Should be fixed now (i. e. I could not reproduce the issue anymore after d9fc998).

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.

Works as expected.

I would change the test dataset to something smaller though.

@@ -176,6 +176,20 @@ def test_empty_groups(self):
self.send_signal(self.widget.Inputs.data, table)
self.assertEqual(2, len(self.widget.boxes))

def test_sorting_disc_group_var(self):
"""Test if subgroups are sorted by their size"""
table = Table("adult")
Copy link
Member

Choose a reason for hiding this comment

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

can you use adult_sample? loading adult takes 0.5s to load on my computer, while loading adult_samples is instant.

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, there really was no reason to use such a large dataset for testing this.

@astaric
Copy link
Member

astaric commented Aug 24, 2018

Thanks

@astaric astaric merged commit 7deb7c3 into biolab:master Aug 24, 2018
@matejklemen matejklemen deleted the enh_boxplot_disc_sorting branch August 24, 2018 18:04
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.

5 participants