The Order of VariableImpacts should be changeable

[mkommend]

The VariableImpactsView calculates and display the variable impacts in descending order. Sometimes an alternate order is preferred, for example order by occurrence or ascending.

[fholzing]

r15626: Added three additional UI-Controls, a Label + ComboBox for ordering (VariableName or ImpactFactor) and a CheckBox for Ascending/Descending

[mkommend]

Review comments from testing:

• I cannot order by variable name.
• I prefer sorting criteria or something similar as label text
• Sort by variable suggests that the impacts are sorted by variable name, where they are actually sorted by occurrence in the data set. A better name is needed Dataset occurrence, Variable position, ....
• Is it possible to add a tooltip to describe the different sort options
• Default sort directions for different sorting criteria? For Impact values the sort order descending is preferable, but on the other hand ascending is better for sort order variable. What do you think?

[fholzing]

r15637: Included the points from the review (added third sorting-possibility, renamed the existing ones, added default-behavior)

[bburlacu]

@fholzing: Hi Florian, could you have a look at #2884 and add the sorting controls also for the ClassificationSolutionVariableImpactsView ?

[fholzing]

alright, i will take a look

[fholzing]

Review comments:

• No reordering after Settings changes
• DropDownList style for all Combo boxes - no free text allowed
• Use enum for sort order instead of int constants

[fholzing]

r15665: Implemented review-issues

[fholzing]

r15673: Implemented review-issues

[fholzing]

Stumbled over Crossthreading-issues

[fholzing]

r15727: Fixed crossthreading-bug Basically tried a new approach with a Backgroundworker.

[fholzing]

r15728: Adapted to naming conventions

[mkommend]

Review of implemented changes:

• Why is a background worker introduced?
• When the view is changed during calculation of the impacts, the progress bar is still displayed

[fholzing]

r15752: Changed back to Task, abort Thread if any other View is shown

[fholzing]

BackgroundWorkers are a convenient way to execute some synchronous code after any asynchronous code has finished. Changed it back to Tasks, so a thread, which can be aborted if any other view is shown, can be obtained and aborted.

[pfleck]

Currently, any exception raised in within the tasks' execution is silently dropped because no results are awaited. I recommend something like

await Task.Run(() => Calculator.CalculateImpacts(...)); 
mainform.RemoveProgress...;

Furthermore, I do not recommend aborting a task by aborting its current (ThreadPool)Thread. Instead we should abort gracefully using a CancellationToken and a manual loop within Task.Run().

[fholzing]

r15796: Added additional logic for progress

[fholzing]

r15797: Rebuild from BackgroundWorker to async await

[fholzing]

r15798: Added additional linebreak for method signature

[fholzing]

r15799: Added textual status update

[fholzing]

r15802: Removed new Features (build.cmd failed)

[pfleck]

Functionality-wise everything works fine.

Only a simple thought: Do we really need the Ascending/Descending checkbox?

Code-Review

• RegressionSolutionVariableImpactsView.cs
  • General
    • Can the IProgress instance variable be made a local variable in UpdateVariableImpact?
  • Constructor
    • remove workaround comment
    • use sortByComboBox.DataSource = Enum.GetValues(typeof(SortingCriteria)) instead, to avoid casting and array handling
    • move default value of sortByCombobox to the other default values below
  • sortByComboBox_SelectedIndexChanged
    • I think a simple assignment (chbx.Checked = curSort == SortCriteria.Occurrence ...) instead of the switch-statement is sufficient.
  • RegressionSolutionVariableImpactsView_VisibleChanged
    • Is this really the best event for firing the cancellation token? Maybe the the control was simply hidden but not closed, thus we lose the current calculation.
    • Checking !IsCancellationRequested is not necessary.
  • UpdateVariableImpact
    • Method is quite long and heavily nested (try -> await -> Task.Run -> Methodcall -> Lambda within method call ... ). Should be refactored to be simpler.
  • UpdateDataOrdering
    • I suggest renaming to UpdateOrdering
    • I would advise against using the rawVariableImpects dictionary's raw order for sorting by occurrence. Maybe you could use a similar mechanism using the originalVariableOrdering from the UpdateVariableImpact.
    • Alternatively, we could also thing about having a set of custom Comperators that handle the different sorting strategies. Those could also be reused for the CalassificationSolutionVariableImpactView.

• RegressionSolutionVariableImpactsCalculator.cs
  • CalculateImpacts
    • I personally do not like having a single callback responsible for progress reporting and cancellation. I would rather split them into a regular IProgress<T> and CancellationToken.
    • Alternatively, we chould introduce a method for calculating the impact of a single variable and perform the whole progress and cancellation within the RegressionSolutionVariableImpactsView.

[fholzing]

r15998: Implemented review

[fholzing]

Replying to fholzing:

r15998: Implemented review

• General (done)
  
• Constructor (done)
  
• sortByComboBox_SelectedIndexChanged (done)
  
• RegressionSolutionVariableImpactsView_VisibleChanged
  -Removed IsCancellationRequested (done)
  -It was the best Event i found to be suitable. I am open to suggestions nonetheless.
  -I also dislike the (often unnecessary) recalculation of the Impacts, i will have another look on it.
  
• UpdateVariableImpact / CalculateImpacts
  -There is another Ticket (#2884) which deals WITH the functionality. I will harmonize them as soon as the other ticket is done.
• UpdateDataOrdering (done)

[fholzing]

r15999: Removed bug smuggled in by implementing the review (Designer)

[fholzing]

r16015: Fixed bug where factory-variables would be ignored

[fholzing]

r16021: Changed default of ReplacementMethod to Shuffle

[fholzing]

r16023: Added UnitTests

[mkommend]

Please remove the introducted unit test in r16023. Everything else looks fine.

[mkommend]

The according variable impact tests has been removed in the related ticket #2904.

[mkommend]

r16435: Merged r15626, r15637, r15665, r15673, r15727, r15728, r15752, r15796, r15797, r15798, r15799, r15802, r15998, r15999, r16015, r16021, r16023 into stable.