wiki:ReviewHeuristicLab3.3.0ApplicationDone

Version 45 (modified by abeham, 11 years ago) (diff)

--

Reviews - HeuristicLab 3.3.0 Application Review - Implemented Review Comments

Reviewer: abeham

Priority: HIGH

  • Creating a new TSP problem and viewing the parameter "Coordinates": The view can be switched to an "OptionalValueParameterView" which adds a button to delete the value. However when clicked an exception is thrown that the value cannot be null. The OptionalValueParameterView should not be selectable (Coordinates is not an optional parameter). This also holds for other parameters like the SolutionCreator.
    • swagner: Changed in r2948.
  • When one creates a new user defined algorithm, the user finds a button called "Create User Defined Algorithm" button on the form. This feels strange.
    • swagner: Set the button to be invisible in r2947.
  • In the user defined algorithm:
    • On the parameter tab: I press "+" to add a new Parameter. I select a ValueParameter, term it "MutationRate" and select DoubleData as type I get a MissingMethodException (Constructor on type ValueParameter not found).
      • swagner: Fixed in r2948.
  • The operator graph view is a bit confusing:
    • Selecting an operator in "Operator Graph" does not bring up the details in the details view.
      • swagner: Fixed in r2949.
    • Should there really be a view host in the operator graph group box? It's confusing being able to switch views there (also because these are always just views on the initial operator which can be viewed with view host in the details group box as well).
      • swagner: Removed view host in r2947.
  • Creating a user defined algorithm from the SGA doesn't reconfigure the SGA operator with the parameters set in the SGA interface.
    • swagner: Fixed as described in ticket #898.
  • The successor parameter reads "Successor: ResultsCollector", "Successor: SubScopesSorter", ... It's more readable if "Successor" was not present all the time (in the operator graph group box).
    • swagner: "Successor" is required to identify an operator parameter. Of course, in the case of ResultsCollector, SubScopesSorter, etc. there is only one operator parameter, but there are other operators which have multiple operator parameters (e.g. branches). If we skip the parameter name, the user will not be able to tell which operator parameter is shown in the tree view.
    • abeham: After some while I got used to it.
  • All Parameters have the same icon, can we have different ones for values and operators?
    • abeham: Implemented this in r3106, fixed a related bug in r3105.
  • There is a bug in the implementation of SimulatedBinaryCrossover.
  • EvolutionStrategy:
    • Prevent self adaptive mutation with non-selfadaptive crossover.
    • BoundsChecking after Crossover and Mutation with optional attribute.
    • Implement SelfAdaptiveCrossover on the algorithm side with two ICrossover parameters.
      • abeham: Implemented in r3183 (the interface thing got killed, since it's ES specific).
  • Disable start button in prewired algorithm (LocalSearch, ...) that expect a problem, when no problem has been loaded. Also prevent start in trajectory based algorithms when e.g. MoveGenerator, etc. is null.
    • swagner: Implemented in SGA in r3188.
    • abeham: Done in r3189.
  • Tabu search should abort when no feasible move found in the neighborhood.
  • Hide choosing of different views when there is only one type of view to select.

Priority: MEDIUM

  • The TSP problem view:
    • How do I set the (allowed) evaluation operator?
      • swagner: Enabled changing values of value parameters in r2948.
    • Why do I want to change e.g. "TSPRoundedEuclideanPathEvaluator" in the textbox "Name" when viewing Parameter "Evaluator"?
      • swagner: In a discussion with abeham it was decided to allow name changes for all operators except for selection, crossover and manipulation operators (or any other operators which can be selected in a standard algorithm or problem view). This was implemented in r3034.
    • BestKnownTour is missing as parameter.
      • swagner: Added import of optimal TSP solutions in r3151.
    • Would be nice to be able to view a "scatter plot" of the coordinates.
      • swagner: Added visualization of the TSP instance in r3153.
    • The TSP path visualization seems to be flipped upside down.
      • swagner: Fixed in r3213.
  • When clicking "start" to start the engine disabling of the user interface elements has similar problems to what we fixed a few revisions ago (ticket #887).
  • The ViewHost menu contains a lot of views which are quite similar. ItemView doesn't display anything, should it be selectable? ParameterView and NamedItemView are very similar. Sometimes there are only very fine differences between the views (e.g. a ParameterView and a NamedItemView).
    • mkommend: Changed in r2992 as described in ticket #902.
  • In my user defined algorithm:
    • After selecting CurrentScope, all further parameters will be displayed as ParameterView and appear as if they cannot be changed. I had to change the view manually in the view host. Sometimes it seems to select the "wrong" default views. It reverts back to ParameterView as default as soon as CurrentScope is clicked again.
      • swagner: Fixed in r2949.
    • InversionManipulator has the "correct" actual name set already (Permutation), however in OrderCrossover the default names are "Parents" and "Child" which have to be changed to Permutation. This isn't obvious right away when the string description of the parameter says: "Child: Child (Permutation)".
      • swagner: Set the actual name of the parents and child parameter to "Permutation" by default in r2947.
    • My initial expectations probably weren't correct regarding the user defined algorithm. The SGA operator is not a complete SGA, but just the main loop (yes we discussed this).
      • swagner: Renamed SGAOperator to SGAMainLoop in r3020.
  • LeftSelector and RightSelector should not be displayed as selection operators in an SGA (there is a related ticket #673).
    • swagner: Replaced LeftSelector and RightSelector by BestSelector and WorstSelector as described in ticket #926.
  • I'd wish there was just a value view as well. Sometimes I want to hide name, description, type, set, remove, ... elements and just display the value (e.g. when viewing the quality chart).
    • swagner: Added VariableValueView in r2989.
  • In the operator graph chart view:
    • Long operator names require higher shapes as the operator name breaks into the next line.
      • mkommend: Implemented with r3169.
    • Scrolling using the mouse wheel looks very odd, if this can't be fixed it better should be disabled.
      • mkommend: Disabled the scrolling using the mouse wheel with r3167.
    • OpenView should be default double click behavior in operator graph chart.
      • mkommend: Implemented with r3184.
    • Sometimes arrows cannot be selected (e.g. when they're pointing directly east).
      • mkommend: Corrected selection of connections with r3175.
    • Just single clicking the successor port with the connector tool connects the box with itself, I don't think this is a desired behavior.
      • mkommend: Fixed with r3181.
    • I would be nice if: As long the space keyboard is pressed the selection mode switches to panning.
      • mkommend: Implemented with r3173.
    • The vertical scroll bar allows scrolling way past the graph.
      • abeham: On second review, this isn't actually an issue.
    • Sometimes one of the ports (in port) remains selected even if clicked somewhere in the white space.
      • mkommend: This is only a minor repainting issue and won't be fixed right now.
    • You can select a connector by drawing a selection rectangle that doesn't cover the connector, but covers the bounding rectangle of the connector.
      • mkommend: This is the default behavior. DiagramEntities are selected if the bounding boxes intersect the selection rectangle.
    • A selected box should change the mouse cursor to the move cursor when hovering over it.
      • mkommend: Implemented with r3186.
  • Viewing different result variables (switching between them) is impossible, because the results view is disabled when running the engine.
    • swagner: Changed in r3275.
  • The GeneticAlgorithm should use ProportionalSelector as default selection operator instead of BestSelector.
    • swagner: Implemented in r3304.

Priority: LOW

  • Sequential engine should probably not be createable. At least currently there's nothing one could do with just an engine.
    • swagner: Removed Creatable attribute in r2924.
  • When creating e.g. a new TSP in the parameter collection view the tool tip doesn't really display the description of the parameter but just a very generic one.
    • swagner: Changed to more specific tool tips in r2947.
  • There is a typo in the description of the parameters (tooltips), it says "it" instead of "in".
    • swagner: Fixed in r2947.
  • "Breakpoint" doesn't explain if it breaks before or after executing the operator.
    • swagner: Added tool tip in r2949.
  • The available parameters view lists just one namespace which is collapsed by default (should be expanded if it's just this one).
    • swagner: Changed in r2953.
  • When you remove the value of the results (such as "BestQuality") the text in the box does not update.
    • swagner: Fixed updating in r2989.
  • The quality chart looks great!
    • "BestKnownQuality" is not displayed currently.
      • swagner: Implemented tracking of best and best known quality values as described in ticket #920.
  • Does it make sense to select no engine in an user defined algorithm?
    • swagner: Disabled that the engine can be set to null in r3214.
  • Does it make sense to change the name of result variables or to remove their values?
    • swagner: Disabled in r3226.

Reviewer: gkronber

Priority: HIGH

Priority: MEDIUM

  • HeuristicLab.Algorithms.TS and HeuristicLab.Algorithms.LS should be renamed to HeuristicLab.Algorithms.TabuSearch and HeuristicLab.Algorithms.LocalSearch.
    • abeham: Resolved.
  • HeuristicLab.Algorithms.SGA should be renamed to HeuristicLab.Algorithms.StandardGeneticAlgorithm.
    • swagner: Renamed HeuristicLab.Algorithms.SGA to HeuristicLab.Algorithms.GeneticAlgorithm as described in #945.
  • Loading the "New Item" dialog takes a long time the first time the "New" action is activated. Probably an issue in the plugin infrastructure.
    • swagner: When opening the new dialog for the first time, all creatable items are instantiated which might take some time (although I do not really notice a long loading time of the new dialog on my machine). At the moment many items are shown in the new dialog for testing purposes (category "Test"). After these items have been removed, the loading time of the new dialog will be much faster. I will remove all [Creatable("Test")] attributes on items in the next version.
    • swagner: Removed all [Creatable("Test")] attributes in r3160.
  • The execution state of an engine (e.g. Execution Time, Log...) is not persisted or restored correctly.
    • swagner: Fixed storing and loading of an engine's execution time and state in r2933. Implemented storing and loading of an engine's log in r3289 (#963).
  • Dropdown-boxes for crossover and mutation operator selection are empty.
    • swagner: They are empty as long as no problem has been selected as an algorithm cannot know a priori which crossover and mutation operators are allowed.

Priority: LOW

  • Are keyboard shortcuts for groupboxes necessary? E.g. Details groupbox and Description shortcuts are conflicting. Value groupbox and Value textbox are conflicting.
    • swagner: Removed keyboard shortcuts for all groupboxes in r2924.
  • Description of RandomCreator should contain a hint about the type of PRNG that is created.
  • ProgrammableOperator: 'Compile' is a normal button but 'Show Generated Code' looks like a label. I think the style should be unified.
    • epitzer: Fixed in r3008.

Reviewer: swinkler

Priority: HIGH

  • After starting an SGA the user is able to add a new problem; the OK button in the following dialog is initially enabled, clicking it causes no (?) action.
    • swagner: Fixed in r2924.
  • Adding a new parameter, for example a ValueParameter<IntData>, leads to an exception.
    • swagner: Fixed in r2948.
  • After starting a SGA with 130 cities TSP and population size 100 the memory consumption is 1.2 GB after approx. 6500 generations.
    • swagner: Memory and runtime performance has been drastically improved in r2932.
  • Adding a paramter to a ProgrammableOperator, for example a ValueParameter<DoubleData>, leads to a null-pointer-exception.
  • (...) As a follow-up, after aborting adding a parameter to a ProgrammableOperator, a PersistenceException is thrown.

Priority: MEDIUM

  • After starting the SGA the current generation index is not displayed; I have to switch to the qualities chart to see the current generation.
    • swagner: Added generations counter to results in r2924.
  • During the run of an algorithm the values displayed for the variables BestQuality, AverageQuality, and WorstQuality flicker.
    • swagner: Fixed in r2924.

Priority: LOW

  • Regarding the description of operators:
    • I would prefer to not to capitalize names of algorithms, i.e., for example I would prefer "... the standard genetic algorithm ..." over "... the Standard Genetic Algorithm ...".
      • swagner: Changed capitalization of algorithm names in r3026.
  • The name of an imported TSP instance is not stored / displayed.
    • swagner: Stored name of TSP instances imported from TSPLIB in r3147.

Reviewer: maffenze

Priority: HIGH

  • When no crossover is selected an error message is thrown which gives no hint about the reason.
    • swagner: Changed in r2924 that a selection and a crossover operator are automatically selected if available to avoid the cryptic error message.
  • Something like a starter package of problems (which do not have to be imported before) should also be included in the release version.
    • swagner: We can include several problems in the setup as it was the case in HL 1.1 or we can offer a set of problems on the same web page where the HL setup can be downloaded. I would prefer the second solution, as it is simpler to realize and the user can easily choose where he wants to save the sample problems. Furthermore, he can download the sample problems again if necessary without having to reinstall HL. However, the first solution has the benefit that the user does not have to download two files. Is the second solution acceptable?
    • swagner: In a discussion with maffenze it was decided to take the second solution (i.e. to offer a set of problems on the HL download page).

Priority: MEDIUM

  • After starting HeuristicLab it is not very obvious that the user has to choose the small new item button in order to have access to the standard methods.
    • swagner: Would it be an acceptable solution to show the new dialog automatically each time the HL 3.3 Optimizer is started? Another solution would be to show a wizard that guides the user through the first steps. If we want to have a wizard, should it show anything else than the available items that can be created?
    • gkronber: I think this is an important point because potential new users can be scared off within one minute if HeuristicLab doesn't have a few simple entry points to begin experimenting. One idea that comes to mind is to open a 'starter document' that has just 3 or 4 large buttons that activate a few basic entry points (e.g. "Genetic Algorithm: Optimize Tours", "Evolution Strategy: Find Minimum of Real-Valued Functions", "Show List of all Demos", "Load Algorithm From File"). Maybe we add an opt-out button for this starter screen. Sure a wizard of the kind "Solve your problem in 4 steps" would be nice but is probably not really necessary.
    • swagner: In a discussion with maffenze it was decided to show a "Start Page" when the optimizer is started that contains initial information how to use HeuristicLab and how to create new algorithms and problems.
    • swagner: A start page has been implemented in r3202. Is this enough to guide new users? Should there be any additional or other information shown on the start page?

Priority: LOW

  • It is not obvious what is meant with the breakpoint option in an operator.
    • swagner: Added tool tip in r2949.

Reviewer: vdorfer

Priority: HIGH

  • First of all: I would regard a user manual with first steps (shall I first create a new TSP or create a new SGA and how can they be combined...) as very important, for me at the first sight it was not really clear how to start.
    • swagner: A start page was added in r3202 which is shown when the optimizer is started. What do you think about it? Is it helpful when starting to work with HeuristicLab? Which additional information should be shown?
    • vdorfer: I like the start page, it gives a good first introduction. Maybe the second and the third example could run a bit longer...
    • swagner: At the moment the samples shown in the start page are just dummies. Meaningful samples will be added right before the release.
  • PersistenceException when saving a TabuSearch with a problem (no matter if TSP, Knapsack,..).
  • PersistenceException when saving an EvolutionStrategy (with and without a problem).

Priority: MEDIUM

  • I would change the stop sign to a pause sign, as it actually pauses the run (maybe also change the tooltip of the start button to "Start/Resume Algorithm"). A stop sign signalizes for me: better not click it, or else I have to start all over again.
    • mkommend: I would also recommend a pause icon instead of the stop icon.
    • abeham: Count me in on that too.
    • swinkler: Me too!
    • swagner: Ok, ok, I got it. Changed in r3225.

Priority: LOW


Reviewer: mkommend

Priority: HIGH

  • Moving a running engine to another dock point (e.g. floating, right) makes the engine unresponsive (no repaint or reaction on mouse clicks).
    • mkommend: This issue was described and fixed in ticket #953.
  • The execution time textbox is only updated after an operation completed. If a long running operation is executed (e.g. SGA + TSP and population size 10000), the textbox is updated infrequently and it looks like as if the application hangs. This should be handled in the GUI by updating the execution time more frequent (every X milliseconds) in a separate thread.
    • mkommend: Changed in r3262.

Priority: MEDIUM

Priority: LOW

  • The new dialog should also react on double clicks on the description column, if this is possible.
    • swagner: Implemented in r3202.


Reviewer: epitzer

Priority: HIGH

Priority: MEDIUM

  • Remove "-" entry from list of available engines if an engine is available or maybe rename to e.g. "no engine".
    • swagner: Changed in r3214.
  • Clicking or double clicking on one of the operators in the operator pane does not do anything this is the first thing useres see and they can't interact with it until rather late in the "development" of a user, when he is ready to create a UserDefinedAlgorithm.
    • Maybe the operators pane should be hidden by default, or only shown once an algorihtm graph is loaded or the click action could give an explanation why nothing happens and that you should drag them over into an operator graph.
    • Another idea, double clicking an operator could add it to the currently visible operator graph. (Probably not so easy to implement)
    • swagner: Changed in r3290 that the operators sidebar is not visible by default.

Priority: LOW

  • Value of Items from the Results pane can be added/changed and deleted.
    • swagner: Disabled in r3226.
  • Engine has to be stopped to be able to click and view results.
    • swagner: Changed in r3275.