[FIX] Slow Rank #2494
[FIX] Slow Rank #2494
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2494 +/- ##
=========================================
+ Coverage 75.08% 75.1% +0.02%
=========================================
Files 329 329
Lines 57773 57636 -137
=========================================
- Hits 43377 43289 -88
+ Misses 14396 14347 -49 |
Orange/widgets/utils/itemmodels.py
Outdated
|
|
||
| def setHorizontalHeaderLabels(self, labels): | ||
| self._headers[Qt.Horizontal] = labels | ||
| def setHorizontalHeaderLabels(self, labels: [str or Variable]): |
There was a problem hiding this comment.
This is not a valid type annotation
Orange/widgets/utils/itemmodels.py
Outdated
|
|
||
| def setVerticalHeaderLabels(self, labels): | ||
| self._headers[Qt.Vertical] = labels | ||
| def setVerticalHeaderLabels(self, labels: [str or Variable]): |
There was a problem hiding this comment.
This is not a valid type annotation
Orange/widgets/utils/itemmodels.py
Outdated
| """ | ||
| A sorting proxy table model that sorts its rows in fast numpy, | ||
| avoiding potentially thousands of calls into | ||
| ``QSortFilterProxyModel.lessThan()`` or any potenrially costly |
There was a problem hiding this comment.
potenrially -> potentially
Orange/widgets/data/owrank.py
Outdated
| header.setSectionResizeMode(header.Fixed) | ||
| header.setFixedWidth(50) | ||
| header.setDefaultSectionSize(22) | ||
| header.setTextElideMode(Qt.ElideMiddle) # QT-BUG |
Orange/widgets/data/owrank.py
Outdated
| selection.append(QItemSelectionRange( | ||
| model.index(row, 0), model.index(row, columnCount - 1))) | ||
|
|
||
| selModel.select(selection, QItemSelectionModel.ClearAndSelect) | ||
|
|
||
| self.setFocus(Qt.OtherFocusReason) # Prevent graying out scores? |
There was a problem hiding this comment.
Do not do this. It steals keyboard focus from the "Best ranked:" spin box while editing.
There was a problem hiding this comment.
Similar was there before: https://github.com/kernc/orange3/blob/177ce2a524d592849ee32463455dc2de6d7b2e6e/Orange/widgets/data/owrank.py#L527-L528
I don't quite get what it was supposed to fix. @thocevar ?
Orange/widgets/utils/itemmodels.py
Outdated
| return self.__sortIndInv[rows] | ||
| return rows | ||
|
|
||
| def invalidate(self): |
There was a problem hiding this comment.
invalidate -> resetSorting?
'invalidate' indicates that something is no longer valid. Is it even needed? It is equivalent to sort(-1).
There was a problem hiding this comment.
I'd rather keep it as I get to override it in Rank's model.
Orange/widgets/utils/itemmodels.py
Outdated
| """The current sort order""" | ||
| return self.__sortOrder | ||
|
|
||
| def mapToSource(self, rows: int or numpy.ndarray or Ellipsis): |
There was a problem hiding this comment.
This only evaluates to def mapToSource(self, rows: int):
Union[int, Sequence[int], type(...)] would be better.
However I would drop the Ellipsis unless you assure the returned array is is a copy or a read only view. Otherwise it makes little sense to keep __sortInd private.
I am also not too thrilled about the name. mapToSourceRows would be better (
I purposefully did not chose this name in TableModel in order to differentiate from QAbstractProxyModel.map{To,From}Source)
Orange/widgets/utils/itemmodels.py
Outdated
| @@ -819,7 +959,7 @@ def addAction(self, action, *args): | |||
| return self.insertAction(-1, action, *args) | |||
|
|
|||
|
|
|||
| class TableModel(QAbstractTableModel): | |||
| class TableModel(AbstractSortTableModel, QAbstractTableModel): | |||
There was a problem hiding this comment.
AbstractSortTableModel is already an QAbstractTableModel.
Building all the QStandardItems for QStandardItemModel and sorting via overridden `__lt__()` (or `lessThan()`) was too slow on datasets with lots of features.
Orange/widgets/data/owrank.py
Outdated
| self.Error.clear() | ||
| self.Information.clear() | ||
| self.Information.missings_imputed( | ||
| shown=self.data is not None and self.data.has_missing()) |
There was a problem hiding this comment.
self.data -> data
self.data is still the old dataset here.
kernc
force-pushed
the
rank-slow
branch
2 times, most recently
from
bdf5532
to
ffeb373
Compare
| # Settings as of version 3.3.5 | ||
| settings = { | ||
| '__version__': 1, |
There was a problem hiding this comment.
Settings in 3.3.5 did not have versions yet. Therefore, __version__ was not present in the dict.
| return str(left_data) < str(right_data) | ||
| # If older settings, restore sort header to default | ||
| # Saved selected_rows will likely be incorrect | ||
| if version is None or version < 2: |
There was a problem hiding this comment.
Why check for None?
When settings dict does not have the __version__ key, 0 is passed as version
There was a problem hiding this comment.
Backwards compatibility. 642e446 😎
Issue
Fixes #2478
Description of changes
Updated owrank to use PyTableModel and its faster sorting, but then I kinda broke something. Then I kinda fixed it by refactoring the whole widget into something I could understand. And it still somewhat works.
Old selection settings won't restore correctly as they were saved as indices of sorted values. Suggestions welcome.
Includes