Moving Transform: Reimplementation

[janezd]

Issue

Widget is outdated, buggy, occasionally suboptimal.

Description of changes

• Fixes #164: Checkbox to keep or remove the leading instances
• Fixes #181 (can be merged, but will work properly only after merging and releasing https://github.com/biolab/orange3/pull/5990)
• Fixes #182: PR implements new, more efficient functions for moving transforms, hence size should no longer be a problem
• Fixes #184: this should no longer happen in the new implementation
• Implements #88: widget is reorganized. Wrt my proposals, the widget still allows multiple transformations for same variable (one may want to see a min and max, for instances); other changes have been implemented.

No longer relevant

• Fixes #88: disable sorting, crash when table is sorted: the widget no longer has such table

Fixed elsewhere

• Extra columns are caused by PyTableModel and fixed in https://github.com/biolab/orange3/pull/5991. This problem no longer relevant for this widget because it no longer uses a table.

Other improvements

• Include categorical variables, but only with a subset of aggregations

To do

• [x] Fix: When numeric attributes are hidden, setting and disabling checkboxes is incorrect because row indices from proxy are used to index transformations for entire domain
• [x] Add filter above listview
• [x] Checkbox to either remove first n-1 rows or fill them with nans (Ref #164)
• [x] Discuss:
  • remove original data columns if windows are non-overlapping?
  • for overlapping windows, keep all original data, and have nans as aggregates in the first width-1 rows? Or have a checkbox to remove those rows?
• [x] Merge functionality of Moving Transform and Aggregate into a single widget (to fix #192)
• [x] Add icon to clear line edit with filter (some kind of ✕ in circle)
• [x] Date conversion is wrong; see e.g. Slovenian accidents data, which changes all dates to one day earlier
• [x] Context handling
• [x] Report ~~ - Settings migrations~~
• [ ] Test that aggregation functions don't fail if window/block is too large; the should return a zero-sized array instead.
• [ ] Tests

[janezd]

Would anybody be interested in checking this widget out? E.g. @ajdapretnar? It still needs testing, unit tests and documentation, but I'd like to have some feedback about its functionality first.

(As for settings migrations - this new widget merges two old ones, and also changes the way settings work, so there'll probably be no migration.)

[ajdapretnar]

I tested quite thoroughly, I think:

• [x] the widget crashes when the data input is disconnected: AttributeError: 'NoneType' object has no attribute 'time_variable'
• [x] if I increase the window beyond the size of the data, widget crashes with: IndexError: too many indices for array: array is 1-dimensional, but 2 were indexed. We should probably set max spin box value to N?
• [x] I probably don't understand what "Consecutive blocks" does. I thought it like a sliding window, but without overlaps. So like [[1, 2, 3, 4, 5], [6, 7, 8, 9, 10], [11, 12, 13, 14, 15]]. But regardless of what I do with block width, I only get a single instance on the output.
• [x] Aggregate time periods outputs "Defined count" by default, without it being selected in the aggregation functions. The UI should correspond to the output, i.e. "Defined count" should be marked in the list and the user should be able to deselect it.
• [x] I set Aggregate time periods. I set Sum as aggregation for my single numeric value. Then I deselect Sum. The widget crashes with: TypeError: descriptor 'discard' for 'set' objects doesn't apply to a 'list' object.
• [x] Since transform options are discrete (radio buttons), wouldn't it be nicer to simply disable the aggregations that don't apply to Consecutive blocks?
• [x] Linear MA crashes with: ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
• [x] Exponential MA crashes with: NameError: name 'n' is not defined
• [x] It would probably be nice allowing the user to discard the original columns after sliding window, too. Like with consecutive blocks.
• [x] Aggregating by minutes doesn't seem to work fine. Observe with Online retails from Datasets.

Not to be just critical. I think the widget is a big improvement over the previous one. I love the layout. We should think about renaming the widget, though, now that it includes more than a single transformation. In pandas, aggregation is, well, aggregation, moving transform is rolling or smoothing. Moving transform currently doesn't imply aggregation. Perhaps Transform Timeseries? Suggestions welcome.

[janezd]

Thanks for thorough testing and apologies for trivial mistakes that I should have discovered myself. (As excuse, I wanted an overview of functionality before writing unit tests, which would find at least some of those. Still, great thanks for testing!)

Here are some comments about thinks I would keep as they are.

if I increase the window beyond the size of the data, widget crashes with: IndexError: too many indices for array: array is 1-dimensional, but 2 were indexed. We should probably set max spin box value to N?

Setting the spin is more dangerous; it essentially introduces a context setting because the spin needs to be reset for new data. Instead, the widget will output a zero-length table and show a warning. This (minus the warning) already happens for some functions.

I probably don't understand what "Consecutive blocks" does. I thought it like a sliding window, but without overlaps. So like [[1, 2, 3, 4, 5], [6, 7, 8, 9, 10], [11, 12, 13, 14, 15]]. But regardless of what I do with block width, I only get a single instance on the output.

Can you give me a more concrete instructions for reproducing this? It works for me.

Aggregate time periods outputs "Defined count" by default, without it being selected in the aggregation functions. The UI should correspond to the output, i.e. "Defined count" should be marked in the list and the user should be able to deselect it.

This is not "Defined count" but "Count". It's the number of instances in a time slot. Checkboxes work for individual attributes, so this option does not belong there. It could be a checkbox in the left part, below the "Aggregate time periods" radio. But unless you strongly object against this column, I'd keep it. It doesn't hurt and it will often be useful.

Since transform options are discrete (radio buttons), wouldn't it be nicer to simply disable the aggregations that don't apply to Consecutive blocks?

Unfortunately no. These checkboxes allow you to control the currently selected attributes. If the user changes transform options, the previously chosen aggregations would still be in effect. The only option would be to disable checkboxes and remove those aggregations (i.e. what you see in the listview). This would be much simpler than current solution, but could lead to situations in which a user would set something up, decide to try different options and lose her settings. That's why aggregations stay but we show a warning.

If I disable checkboxes, I only prevent the user from deselecting options that have to be deselected to avoid the warning.

This problem made me think whether I should split this into three widgets ... but I decided against because they'd be basically the same.

[ajdapretnar]

Instead, the widget will output a zero-length table and show a warning.

Agree.

But unless you strongly object against this column, I'd keep it.

I am not saying it is useless, but it is counterintuitive to me to have something on the output that I didn't select in the settings. I don't strongly object, but it might be annoying for the user who wishes to keep exactly what she sets (thus requiring an additional Select Columns to get rid of it).

That's why aggregations stay but we show a warning.

Ok.

As for the Consecutive blocks, the issue is with the spin box. It looks like it takes the wrong spin box value, that is from the Sliding window not its own. I think this is the culprit: https://github.com/janezd/orange3-timeseries/blob/cb755dc94051912a496c88f04ec1cc75984ed14b/orangecontrib/timeseries/widgets/owmovingtransform.py#L478

[codecov-commenter]

Codecov Report

Merging #197 (b65306b) into master (bad9404) will decrease coverage by 3.38%. The diff coverage is 41.07%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   73.20%   69.81%   -3.39%     
==========================================
  Files           7        7              
  Lines         821      858      +37     
  Branches      161      163       +2     
==========================================
- Hits          601      599       -2     
- Misses        168      210      +42     
+ Partials       52       49       -3     

Impacted Files	Coverage Δ
orangecontrib/timeseries/functions.py	68.29% <41.07%> (-8.80%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bad9404...b65306b. Read the comment docs.