wiki:ReviewHeuristicLab3.3.0Application

Version 344 (modified by gkronber, 12 years ago) (diff)

removed fixed issues.

Reviews - HeuristicLab 3.3.0 Application Review

Review comments which have been implemented are listed here.

Reviewer: abeham

Priority: HIGHEST

  • Adding a ValueParameter<T> in a VariableCreator and selecting a generic type e.g. ItemList<T> fails because no choice can be made on the generic type of the ItemList (ticket #42).
    • abeham: swagner fixed this, but there was still an issue with an ItemList<IItemList<T>> which cannot be assigned a value
  • Generations and MaximumGenerations parameters in MichalewiczNonUniform(One|All)PositionsManipulator are not wired
    • abeham: set them to default to "Generations" in r3677. This is not a complete fix, but will work for now.

Priority: HIGH

  • When there is an error in one of the item's constructors, the TypeSelector dialog (when pressing the "new" button) will open the first time, but stop listing items after encountering the item with the error. The second time the TypeSelector is shown through the "new" button, the optimizer crashes with an exception.
  • Remember the optimizer to start maximized if it was maximized when last closed
  • Creating a UserDefinedAlgorithm from any existing wired algorithm before adding a new problem is not the best idea. There should be some kind of warning to the unexperienced user that he's entering dangerous territory if he clicks the prominent, big and inviting button "Create User Defined Algorithm".
  • Opening a file that is not a hl file should display a nicer dialog than the PersistenceException that complains about a missing typecache.xml
  • Operator graph view
    • The boxes grow in size when the text gets larger, but they do not shrink again when the text gets smaller
    • Sometimes the arrow is not drawn when connecting two operators, or it's drawn wrong
    • The red square surrounding a port still remains selected sometimes after creating a connection
  • Operator graph tree-view
    • The breakpoint icon looks a bit big
  • When the TSP importer is used after pausing the execution, the engine should be reset.
  • Would be good to be able to discriminate runs based on the problem type and instance when analyzing in a run collection
  • Could be interesting to plot PercentTabu over time

Priority: MEDIUM

  • Every IDeepCloneable item needs to have a public default constructor. If Activator.CreateInstance in DeepCloneable.Clone is given a second parameter "true", non-public constructors would also be used.
    • swagner: See ticket #922.
  • Adding a new parameter through the GUI and choosing a ConstrainedValueParameter does not make sense as the set of valid values cannot be defined anywhere in the GUI. The parameter will never be able to take a value. Additionally, switching the view for this ConstrainedValueParameter to a ValueParameterView and setting a value there leads to an exception. The ValueParameterView should not be a view for the ConstrainedValueParameter.
  • Adding a new ValueParameter<T> could create a default value if T is instantiable. For most ValueParameters you will want to have a value upon creation.
  • The tabs don't change when you drag something over them (e.g. you have parameters tab open and drag a problem in: So you have to cancel drag, switch to problem tab and redo the drag).
  • Photoshop cannot parse the bitmap file format generated by the screenshot tool of the operator graph view. Is this a standard format?
  • In the operator graph view it's possible to add operators to the selection by (ctrl|shift)-clicking, but not removing them again through another (ctrl|shift)-click.
    • mkommend: This is the default behavior of the Netron library. As far as I understand the source it is related to selection of grouped / stacked items.
      • abeham: If that means it's impossible to change please delete the whole issue. I'm moving it down in the priority list.
  • Placing two operators in the operator graph view above each other and left-clicking selects the operator on top, but right-clicking opens the context menu on the bottom one

Priority: LOW

  • The quality chart looks great!
    • We should probably use the same colors we had in 1.1 for best/average/worst qualities.
    • Would be good to be able to hide certain lines.
    • At some point we'll certainly need export to png, eps, etc. functionality (would be great to be able to specify resolution and width/height).
  • The DataTableValuesCollector says it can only display double values, why not ints?
    • abeham: In r3489 the collector was expanded to display IEnumerable<DoubleValue>, this could further be extended to ints or probably solved through the unified INumber
  • I got a very cryptic "Operation is not valid due to the current state of the object" exception at one point building the TS operator (as discussed Parameters should have an optional attribute that says they should throw an error in case ActualValue is null after retrieving).
  • Improve description of SearchIntervalFactor in BreederGeneticAlgorithmManipulator.
  • EvolutionStrategy:
    • Add parameter to ES to indicate if 1/5 success rule or sigma-self adapation should be used.
  • Name in docking tab is not synchronized with the item name it displays.

Open Discussions

  • The operator graph view is a bit confusing:
    • The flow chart is very nice, but sometimes arrows are a bit wild and going backwards which creates a not so nice strikethrough appearance. Would be good to have non-direct arrows as well.
      • swagner: In a discussion with maffenze it was decided to address this issue after the release of HeuristicLab 3.3.0.
  • The OperatorGraphVisualization doesn't respect operator collections (e.g. SequentialSubScopesProcessor).
    • mkommend: This feature will rarely be used by a standard user and it will not be fixed right now. Although special shapes for MultipleCallOperators are planned.
  • In TypeSelector filter types that don't have a constructor (like currently PathTSPTour).
    • swagner: Generally all items should provide a parameterless constructor. I implemented one for PathTSPTour in r3139.
  • Run collection: By drag-drop runs can be duplicated (also reported by vdorfer).
    • swagner: Dropping runs into a run collection is allowed to enable users to copy/move interesting runs into a run collection and to analyze these runs later on. If we disable drop operations on run collections to avoid the duplication of runs, this will not be possible anymore. Do you think that this feature is not required?
      • abeham: In my opinion the algorithm view should not be used as analyzer for runs other than the ones it created. It would be better to allow creating a new run collection and drop interesting runs there for further analysis.
  • In the operator graph chart view:
    • Top-down layouting would be better instead of left-right for two reasons:
      1. The boxes are usually much wider than high and thus a left-right layout makes the graph even longer.
      2. A top-down graph better fits the paper orientation in a publication.
      • mkommend: This will be implemented with the other changes regarding the layout of the operator graph.
  • Focus is not removed from a textbox if one clicks anywhere in the "gray" area.
    • swagner: This is the behavior of Windows Forms.
    • abeham: I mentioned this because I want to validate the control by clicking out of the text box and thus losing the focus. Maybe this can be achieved by allowing focus on the control or form. I think by default the "gray area" doesn't accept focus.
  • The view host icon should act like a button that when single-clicked opens the context-menu, when hovering over it, there should be a frame around it like a button so that it's clear that it can be clicked.
    • swagner: When hovering over the view host icon a tool tip is displayed that explains how the different views can be selected.
    • abeham: I'm still not fully happy with the view host icon, but it's probably the least priority thing right now
  • Some operators have a Parameter "CurrentScope" that cannot be changed in any way, is there a necessity to display this parameter?
    • swagner: The scope parameter "CurrentScope" indicates that the current scope (i.e. its sub-scopes) is manipulated by an operator. It is shown in order to give the user comprehensive information about what an operator does. Maybe this is not really helpful. Should I remove scope parameters in general? In the code of an operator they are not really required, as the current scope can always be accessed via the current execution context.
    • swagner: In a discussion with maffenze it was decided to keep scope parameters.
    • abeham: I think more parameter are just more confusing and description of the internal mechanisms of an operator are better displayed in the description text. I'll vote for removal of the current scope parameters, but I'll move this to low priority for now.

Reviewer: gkronber

Priority: HIGHEST

Priority: HIGH

  • Read-only parameters would be nice (similar to the data items of HeuristicLab.Data)
  • Importing new data into a symbolic regression problem for which a solution already exists throws an exception.
  • Using an architecture manipulation operator when the number of function defining branches or function arguments is set to 0 leads to an exception.

Priority: MEDIUM

  • It seems strange now that operators have the property Breakpoint. It is more usual to set breakpoints on invocations/calls of operators. Also isn't the engine responsible for breakpoints?
    • swagner: As in HL 3.2 the Breakpoint property is implemented in operators and can be changed in the GUI of an OperatorGraph. Additionally, I decided to add a checkbox to each operator in order to be able to mark breakpoints there as well (this was not possible in HL 3.2). If an operator is marked as breakpoint, an engine stops its execution after this operator has been processed. I agree that it would be more intuitive to store it in some other location, which operators are currently breakpoints. However, I do not immediately know where. Any suggestions?

Priority: LOW

  • Relative errors should be displayed as percentage values and rounded to ~4 digits after the decimal point.

Reviewer: swinkler

Priority: HIGHEST

  • After defining a batch run (GA, TSP, 5 repetitions) and dragging it into an experiment, the "play" button of the experiment is still disabled.
    • Follow-up: The play-button is enabled after adding a new algorithm (and adding a problem to this algorithm).
  • HeuristicLab crashes fatally when I do the following: I run a GA solving the TSP ch130 problem several times, filter the results and let the results be displayed in a bubble chart (showing the best qualities). Then I start the GA with another problem (a Knapsack problem) without clearing the results, and then HL crashes.
    • gkronber: This might be caused by my changes to the bubble chart today (r3707).

Priority: HIGH

  • Tabu Search: Adding a new symb reg problem disables the play button; that's ok, I suppose. But after adding a Knapsack problem and then a symb reg problem the play button is enabled, and that leads to an exception.

Priority: MEDIUM

  • Algorithm testing: I have tested an OS-GA solving the TSP130 in HL2.5 (legacy plugins) and HL 3.3; pop. size: 100, success rate: 0.8, comparison factor: 0.0, max. selection pressure: 100, insertion mutation, CX crossover in HL2.5, CyclicCrossover2 in HL3.3. The resullts differ significantly: In HL 2.5 the produced solutions' quality is in the range 13,000 - 18,000, in HL3.3 it is in the range 20,000 - 24,000. Maybe I have overseen something?
  • There is no Help menu, and also no About dialog.
  • I am not completely happy about being asked if I want to clear all runs after each click on the reset button. I would prefer to have a button for this in the runs section.
  • I like the graphical display of the twodimensional test function solutions - I would suggest adding the display of min and max displayed values.
  • The details of the GP grammar currently used are not displayed.

Priority: LOW

  • Regarding the description of operators:
    • As the references to articles and books are incorporated in grammatically meaningful sentences (which I like a lot!) I would not use full stops (.) within references; I would use semicola (;) instead.
  • In the symbReg problem parameter "SymbolicExpressionTreeInterpreter:" I would remove the ":"

Open Discussions

  • Could it be reasonable to display a special icon for parameters that are not yet set properly (e.g., with a red exclamation mark etc.)?
    • swagner: This will be quite hard as it is not easy to decide programmatically, if a parameter is properly set or not. But we can think about it. Maybe we can introduce constrained parameters? Although it will be a lot of work to enable users to define constraints of parameters in the GUI (for example when adding a new parameter to a CombinedOperator).
  • On a screen with resolution 1280x800 I can for example not change the group size of tournament selection because the text box is not visible (unless the OptionalValueParameter View is chosen).
    • swagner: Can you give me some hints where we can save space? Which information/controls might not be necessary?
    • swinkler: As names and data types cannot be edited in the details sections of parameters, I would suggest combining these two into one TextBox and showing data types in brackets (as it is done in the list box shown left, e.g.).
  • IMHO the standard selection operators in gender specific selection should be flipped; this would more reflect behavior in nature. The reason for this is that these selectors do not specify how fe/male individuals select their mates, but rather how they are selected.

Reviewer: maffenze

Priority: HIGHEST

Priority: HIGH

Priority: MEDIUM

Priority: LOW


Reviewer: vdorfer

Priority: HIGHEST

  • When clicking on the open file dialog before loading of the start page is finished the program crashes without exception.
    • swagner: Could not reproduce this exception in r3561. Does it always occur? May you give me some additional hints?
    • vdorfer: It is reproducible in the release candidate and in r3561 - if not started in debug mode of VS! The open command has to be made in the first half of the loading bar.
    • abeham: Could not reproduce this exception in r3695.

Priority: HIGH

Priority: MEDIUM

  • Is it really intended that runs in the run tab can be copied (through drag&drop)?
    • swagner: Dropping runs into a run collection is allowed to enable users to copy/move interesting runs into a run collection and to analyze these runs later on. If we disable drop operations on run collections to avoid the duplication of runs, this will not be possible anymore. Do you think that this feature is not required?
  • The "Show Algorithm" button in the run tab opens a new tab containing the algorithm but no runs and no results - shouldn't it rather show the problem tab in the current algorithm?
    • swagner: A new algorithm view is shown as the algorithm stored in a run is cloned when "Show Algorithm" is clicked to avoid any manipulation of the algorithm that created the run. The algorithm stored in a run does not contain any runs or results as this would require too much memory.
  • In the results view I would show by default the quality data table (instead of first having to stop (pause) the algorithm, select the data table and resume the algorithm). When the tour visualization is done, this would be best to show by default.
    • swagner: Showing results by default is critical, as the continuous redraw of a result's value (e.g. quality chart, tour visualization) might consume a lot of runtime. Therefore the user should have to select a result manually, if he wants to look at a result.
  • A "Do you want to save your work" question is missing when clicking the "x" of an open SGA, TSP, ...
  • Data Analysis Problem: Trying to import a csv file which is opened in Excel leads to an IOException (cannot access file, used by another process)
  • SASEGASA - Results Tab: The generation counter does not increase by 1 but jumps e.g. from 0 to 60 then to 71 ....
    • abeham: That's because each village is run until it terminates. If all villages are terminated the generations are updated displaying the maximum of the generation that you see jumping and the maximum of the generations of the villages. The name "Generations" is probably misleading. I am open for suggestions.
    • swinkler: I think "generations" is ok; we're not used to nonlinear progresses of the generations counter, but in this case it's ok.

Priority: LOW

  • The comments in the generated code of a ProgrammableOperator are German.
    • epitzer: This is a feature of the .NET runtime and depends on the installed operating system language.
  • The Solution Visualization icon of an Artificial Ant Problem is not the same as in other problems

Reviewer: mkommend

Priority: HIGHEST

  • Experiment: If rerunning an experiment without clearing the contained runs, the included batchruns are only execute once instead of 10 times.

Priority: HIGH

  • Parameter checks should be added to every algorithm. E.g. it should not be possible to start an GA/OSGA with negative or zero population size. Neg. population size results in an ArithmeticOverflowException in the selection operator, because arrays of negative size could not be allocated.
  • Negative mutation rates or maximum generations do not produce an error, but are meaningless and should IMHO also be checked.
  • Experiment - BatchRun: When stopping a BatchRun explicitly during the execution of an experiment, it often results in an error message stating that the BatchRun cannot be stopped while in stopped stated. I guess this is somehow related to the asynchronous GUI updating / executing of engines.
  • The start page does not respect the loading cursor.

Priority: MEDIUM

  • Clipboard: It would be very nice to be able to rename items contained in the clipboard.
  • CollectionViews: The details views of CollectionViews should also be cached to remember the actual state of the views.

Priority: LOW

  • GP Solution: The usage of views for sym reg GP solutions should be improved (e.g. by using tabs).

Reviewer: epitzer

Priority: HIGHEST

Priority: HIGH

  • A Help menu should be available, maybe also an about dialog.

Priority: MEDIUM

  • Problem parameters that cannot be changed by the user could be hidden.
    • i.e. Maximation is always the first parameter but can rarely be changed.
    • swagner: In a discussion with maffenze it was decided not to hide parameters. The parameters collection of a problem should show all parameters of a problem so that a user sees and understands how the underlying problem object looks like. This should help users when they start to implement own problems.
  • The [+] Button in the ValueParameterView should also be disabled if the type of the parameter cannot be changed.
    • swagner: Even if the type of a value parameter cannot be changed, it might be necessary to be able to exchange the whole value object. Therefore the [+] button should be enabled in every case.
  • New Item Dialog: Algorithm and Problem items should be listed before the advanced items in "Algorithm Design".
    • swagner: In a discussion with maffenze it was decided to keep alphabetical sorting of creatable groups in the new item dialog. Alphabetical sorting is used in many places throughout the whole application and it might be irritating for a user if we change this in the new item dialog. Furthermore, it depends on a user which creatable groups are considered to be more important.
  • Parameters of sub-operators that are not changeable by the user should be hidden (e.g. lookup parameter).
  • Graph View
    • Icon for expanding/collapsing details is rather confusing (undo/redo), should be +/- imho
    • A zoom rectangle tool would be nice
    • panning while pressing the middle mouse button would be nice too
    • display updates while scrolling and panning are rather slow

Priority: LOW

  • Show results tab when engine is started, set a default item that is selected too (e.g. quality chart).
    • swagner: Showing results by default is critical, as the continuous redraw of a result's value (e.g. a quality chart) might consume a lot of runtime. Therefore the user should have to select a result manually, if he wants to look at a result.
  • Hierarchical grouping of operators by namespace in operator tab (instead of just by plugin) or maybe just remove the intial "HeuristicLab" as this is common for all of them and makes it harder to scan for the right plugin/namespace.
    • swagner: In a discussion with maffenze it was decided not to change the way how operators are grouped and displayed in the operators sidebar. Reasons are that grouping hierarchically by namespaces might lead to deep trees that have to be opened to get an operator and that the initial "HeuristicLab" in each plugin name might not be common for all plugins if an external user develops an own plugin.

Reviewer: svonolfen

Priority: HIGHEST

Priority: HIGH

Priority: MEDIUM

Priority: LOW

  • The view flickers, because double buffering is not enabled.

Other Reviewers

Priority: HIGHEST

Priority: HIGH

Priority: MEDIUM

  • It would be better to have scrollbars instead of infinitely small controls. Maybe this could be achieved by setting the MinimumSize for each View/Control.

Priority: LOW


Attachments (8)