Free cookie consent management tool by TermsFeed Policy Generator

Opened 6 years ago

Closed 5 years ago

#2871 closed enhancement (done)

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 (44)

comment:1 Changed 6 years ago by fholzing

  • Status changed from new to accepted

comment:2 Changed 6 years ago by fholzing

  • Cc bwerth added

comment:3 Changed 6 years ago by fholzing

  • Version set to trunk

comment:4 Changed 6 years 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 6 years ago by fholzing

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

comment:6 Changed 6 years 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 6 years ago by fholzing

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

comment:8 Changed 6 years ago by fholzing

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

comment:9 Changed 6 years 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 6 years ago by fholzing

alright, i will take a look

comment:11 Changed 6 years 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 6 years ago by fholzing

r15665: Implemented review-issues

Last edited 6 years ago by fholzing (previous) (diff)

comment:13 Changed 6 years ago by fholzing

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

comment:14 Changed 6 years ago by fholzing

r15673: Implemented review-issues

comment:15 Changed 6 years ago by fholzing

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

comment:16 Changed 6 years ago by fholzing

Stumbled over Crossthreading-issues

comment:17 Changed 6 years ago by fholzing

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

Last edited 6 years ago by fholzing (previous) (diff)

comment:18 Changed 6 years ago by fholzing

r15728: Adapted to naming conventions

comment:19 Changed 6 years ago by fholzing

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

comment:20 Changed 6 years 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 6 years ago by fholzing

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

comment:22 Changed 6 years 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 6 years ago by fholzing (previous) (diff)

comment:23 Changed 6 years ago by fholzing

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

comment:24 Changed 6 years ago by fholzing

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

comment:25 Changed 6 years 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 6 years ago by fholzing

r15796: Added additional logic for progress

comment:27 Changed 6 years ago by fholzing

r15797: Rebuild from BackgroundWorker to async await

comment:28 Changed 6 years ago by fholzing

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

comment:29 Changed 6 years ago by fholzing

r15798: Added additional linebreak for method signature

comment:30 Changed 6 years ago by fholzing

r15799: Added textual status update

comment:31 Changed 6 years ago by fholzing

r15802: Removed new Features (build.cmd failed)

comment:32 Changed 6 years 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 6 years ago by fholzing

r15998: Implemented review

comment:34 in reply to: ↑ 33 Changed 6 years 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 6 years ago by fholzing (previous) (diff)

comment:35 Changed 6 years ago by fholzing

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

comment:36 Changed 6 years ago by fholzing

r16015: Fixed bug where factory-variables would be ignored

comment:37 Changed 6 years ago by fholzing

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

comment:38 Changed 6 years ago by fholzing

r16021: Changed default of ReplacementMethod to Shuffle

comment:39 Changed 6 years ago by fholzing

r16023: Added UnitTests

comment:40 Changed 5 years 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 5 years ago by mkommend

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

comment:42 Changed 5 years ago by mkommend

  • Status changed from reviewing to readytorelease

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

comment:44 Changed 5 years ago by mkommend

  • Resolution set to done
  • Status changed from readytorelease to closed
Note: See TracTickets for help on using tickets.