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] Test and Score: Sort numerically, not alphabetically #3951

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Jul 30, 2019

Fixes #3947 by using a QSortFilterProxy that sorts numerically, if column values can be converted to numbers, or case-insensitive, if they can't.

Includes
  • Code changes
  • Tests

@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #3951 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3951      +/-   ##
==========================================
- Coverage   85.22%   85.13%   -0.09%     
==========================================
  Files         382      378       -4     
  Lines       67595    67159     -436     
==========================================
- Hits        57606    57176     -430     
+ Misses       9989     9983       -6

@ales-erjavec
Copy link
Contributor

Or maybe just undo b419679 and go on from there.

diff --git a/Orange/widgets/evaluate/owtestlearners.py b/Orange/widgets/evaluate/owtestlearners.py
index 139f2cb9c..9a2d77de9 100644
--- a/Orange/widgets/evaluate/owtestlearners.py
+++ b/Orange/widgets/evaluate/owtestlearners.py
@@ -533,7 +533,7 @@ class OWTestLearners(OWWidget):
                 for stat, scorer in zip(stats, self.scorers):
                     item = QStandardItem()
                     if stat.success:
-                        item.setText("{:.3f}".format(stat.value[0]))
+                        item.setData(float(stat.value[0]), Qt.DisplayRole)
                     else:
                         item.setToolTip(str(stat.exception))
                         if scorer.name in self.score_table.shown_scores:
diff --git a/Orange/widgets/evaluate/utils.py b/Orange/widgets/evaluate/utils.py
index ebe060327..6caa6811b 100644
--- a/Orange/widgets/evaluate/utils.py
+++ b/Orange/widgets/evaluate/utils.py
@@ -109,6 +109,12 @@ class ScoreTable(OWComponent, QObject):
             size = super().sizeHint(*args)
             return QSize(size.width(), size.height() + 6)
 
+        def displayText(self, value, locale):
+            if isinstance(value, float):
+                return "{:.3f}".format(value)
+            else:
+                return super().displayText(value, locale)
+
     def __init__(self, master):
         QObject.__init__(self)
         OWComponent.__init__(self, master)

@janezd janezd force-pushed the test-score-sorting branch from bb87440 to 2644ffc Compare July 31, 2019 15:34
@janezd
Copy link
Contributor Author

janezd commented Jul 31, 2019

Thanks @ales-erjavec. I reverted the changes in b419679, as you proposed.

I still kept a (now simpler) proxy because I like putting the failed tests at the bottom.

@thocevar thocevar self-assigned this Aug 2, 2019
@thocevar
Copy link
Contributor

thocevar commented Aug 2, 2019

Sorting has a problem with np.nan because it is a float.

A stable sort would be nice for sorting by multiple criteria (e.g. if the first criteria reaches the max value). Sorting by name and then by some constant column does not retain the alphabetical order. I don't know why this doesn't work out of the box.

This would also solve the problem with a changing order of failed tests. If you have several, they change positions when going from ascending to descending (but stay in place from descending to ascending).

@janezd
Copy link
Contributor Author

janezd commented Aug 2, 2019

Sorting has a problem with np.nan because it is a float.

There should be no nans in this table. If there are (but I haven't seen them yet), this is a different bug; they have to be replaced by errors by the widget.

A stable sort would be nice for sorting by multiple criteria (e.g. if the first criteria reaches the max value).

Sorting is done by QSortFilterProxyModel. My class provides just the comparison, I can't control the algorithm.

This would also solve the problem with a changing order of failed tests.

It would be nice to keep the order, but it's not such a problem. :)

@thocevar
Copy link
Contributor

thocevar commented Aug 6, 2019

There should be no nans in this table. If there are (but I haven't seen them yet), this is a different bug; they have to be replaced by errors by the widget.

True, but as it is a part of the utils, it would be nice if it handled this additional case (besides "" and None) that might come up somewhere else. Actually, anything that is not a float could fall into the same bottom category.

Sorting is done by QSortFilterProxyModel. My class provides just the comparison, I can't control the algorithm.

I've just checked that QSortFilterProxyModel behaves as a stable sort in the Predictions widget. Now I can't find the case, where it doesn't, so maybe it does. :)

@janezd janezd force-pushed the test-score-sorting branch from 2644ffc to 0887a2a Compare August 17, 2019 17:09
@janezd
Copy link
Contributor Author

janezd commented Aug 17, 2019

There should be no nans in this table.

True, but as it is a part of the utils, it would be nice if it handled this additional case (besides "" and None) that might come up somewhere else. Actually, anything that is not a float could fall into the same bottom category.

ScoreModel is supposed to contain scores, and these cannot (should not) be nan. But you're right: it's better to support them to avoid problems in the future and particularly to avoid bugs if somebody copies this code or moves it to some more general utils module. Done. The changed code is also cleaner.

@thocevar thocevar merged commit bf6edbe into biolab:master Aug 19, 2019
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.

Bug: Test and Score Evaluation results sort alphabetically
3 participants