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

OWRank: sort NaNs last; fix sort indicator #2618

Merged
merged 4 commits into from
Sep 27, 2017
Merged

Conversation

kernc
Copy link
Contributor

@kernc kernc commented Sep 25, 2017

Issue

Fixes #2617

Description of changes

Always sort NaNs last. Implemented by exposing reimplementable AbstractSortTableModel._argsortData(). Also fix header indicator by sorting via view.horizontalHeader().setSortIdicator() instead of its model.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Sep 25, 2017

Codecov Report

Merging #2618 into master will increase coverage by <.01%.
The diff coverage is 94.73%.

@@            Coverage Diff            @@
##           master   #2618      +/-   ##
=========================================
+ Coverage   74.99%     75%   +<.01%     
=========================================
  Files         331     331              
  Lines       58076   58104      +28     
=========================================
+ Hits        43555   43581      +26     
- Misses      14521   14523       +2

@ajdapretnar
Copy link
Contributor

ajdapretnar commented Sep 25, 2017

This looks good.

Would you care to check this error, too?

orange/orange3/Orange/widgets/settings.py:380: UserWarning: Could not read defaults for widget <class 'Orange.widgets.data.owrank.OWRank'>
The following error occurred:

'headerState'
.format(self.widget_class, ex))

@ajdapretnar
Copy link
Contributor

Still some comments.

  1. When I sort by, say, Info. gain and chance the data set, the sorting doesn't pertain. I have to re-sort again even though there's an arrow indicating the sorted column.
  2. Could we turn off sorting by # or is this not possible with the model?

@kernc
Copy link
Contributor Author

kernc commented Sep 26, 2017

Fixed.

Could we turn off sorting by # or is this not possible with the model?

Sure it's possible. But one may wish to split scores by variable type, or to keep discrete variables with the number of values within a certain threshold. My vote is against.

@ajdapretnar
Copy link
Contributor

But one may wish to split scores by variable type, or to keep discrete variables with the number of values within a certain threshold. My vote is against.

Ok. It is just a little ugly when sorting by #. Perhaps add a tad of extra width then?

screen shot 2017-09-26 at 15 29 58

@@ -417,13 +424,14 @@ def updateScores(self):

self.ranksModel.wrap(model_array.tolist())
self.ranksModel.setHorizontalHeaderLabels(('#',) + labels)
self.ranksView.setColumnWidth(0, 30)
self.ranksView.setColumnWidth(0, 40)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajdapretnar Wide enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but now it looks annoyingly wide. 😆 I'm such a brat. I'll merge this as it is a great solution! 👌

@ajdapretnar
Copy link
Contributor

Ok, I said I would merge this, but codecov is complaining...

@jerneju jerneju merged commit 9ade0d7 into biolab:master Sep 27, 2017
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.

Rank: incorrect ranking
4 participants