Opened 11 months ago

Last modified 63 minutes ago

#2871 reviewing enhancement

The Order of VariableImpacts should be changeable

Reported by: mkommend Owned by: mkommend
Priority: medium Milestone: HeuristicLab 3.3.16
Component: Problems.DataAnalysis.Views Version: trunk
Keywords: Cc: bwerth

Description

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

Change History (41)

comment:1 Changed 11 months ago by fholzing

  • Status changed from new to accepted

comment:2 Changed 11 months ago by fholzing

  • Cc bwerth added

comment:3 Changed 11 months ago by fholzing

  • Version set to trunk

comment:4 Changed 11 months ago by fholzing

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

comment:5 Changed 11 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from accepted to reviewing

comment:6 Changed 11 months ago by mkommend

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

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?

comment:7 Changed 11 months ago by fholzing

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

comment:8 Changed 11 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing

comment:9 Changed 11 months ago by bburlacu

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

comment:10 Changed 11 months ago by fholzing

alright, i will take a look

comment:11 Changed 11 months ago by fholzing

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

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

comment:12 Changed 11 months ago by fholzing

r15665: Implemented review-issues

Last edited 11 months ago by fholzing (previous) (diff)

comment:13 Changed 11 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing

comment:14 Changed 11 months ago by fholzing

r15673: Implemented review-issues

comment:15 Changed 11 months ago by fholzing

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

comment:16 Changed 11 months ago by fholzing

Stumbled over Crossthreading-issues

comment:17 Changed 10 months ago by fholzing

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

Last edited 10 months ago by fholzing (previous) (diff)

comment:18 Changed 10 months ago by fholzing

r15728: Adapted to naming conventions

comment:19 Changed 10 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing

comment:20 Changed 10 months ago by mkommend

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

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

comment:21 Changed 10 months ago by fholzing

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

comment:22 Changed 10 months ago by 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.

Last edited 10 months ago by fholzing (previous) (diff)

comment:23 Changed 10 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing

comment:24 Changed 10 months ago by fholzing

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

comment:25 Changed 10 months ago by 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().

comment:26 Changed 10 months ago by fholzing

r15796: Added additional logic for progress

comment:27 Changed 10 months ago by fholzing

r15797: Rebuild from BackgroundWorker to async await

comment:28 Changed 10 months ago by fholzing

  • Owner changed from fholzing to pfleck
  • Status changed from assigned to reviewing

comment:29 Changed 10 months ago by fholzing

r15798: Added additional linebreak for method signature

comment:30 Changed 10 months ago by fholzing

r15799: Added textual status update

comment:31 Changed 10 months ago by fholzing

r15802: Removed new Features (build.cmd failed)

comment:32 Changed 7 months ago by pfleck

  • Owner changed from pfleck to fholzing
  • Status changed from reviewing to assigned

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.

comment:33 follow-up: Changed 5 months ago by fholzing

r15998: Implemented review

comment:34 in reply to: ↑ 33 Changed 5 months ago by 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 (#2904) which deals with the functionality. I will harmonize them as soon as the other ticket is done.
  • UpdateDataOrdering (done)
Last edited 5 months ago by fholzing (previous) (diff)

comment:35 Changed 5 months ago by fholzing

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

comment:36 Changed 5 months ago by fholzing

r16015: Fixed bug where factory-variables would be ignored

comment:37 Changed 5 months ago by fholzing

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing

comment:38 Changed 5 months ago by fholzing

r16021: Changed default of ReplacementMethod to Shuffle

comment:39 Changed 5 months ago by fholzing

r16023: Added UnitTests

comment:40 Changed 2 months ago by mkommend

  • Owner changed from mkommend to fholzing
  • Status changed from reviewing to assigned

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

comment:41 Changed 63 minutes ago by mkommend

  • Owner changed from fholzing to mkommend
  • Status changed from assigned to reviewing
Note: See TracTickets for help on using tickets.