= [wiki:Reviews Reviews] - HeuristicLab 3.3.0 Application Review = Review comments which have been implemented are listed [wiki:ReviewHeuristicLab3.3.0ApplicationDone here]. == Reviewer: abeham == === Priority: HIGH === * 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. * Adding a `ValueParameter` in a `VariableCreator` and selecting a generic type e.g. `ItemList` fails because no choice can be made on the generic type of the `ItemList` (ticket #42). * The !OperatorGraphVisualization doesn't respect operator collections (e.g. `SequentialSubScopesProcessor`) * In `TypeSelector` filter types that don't have a constructor (like currently `PathTSPTour`) * 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 * Disable start button in prewired algorithm (localsearch, ... ) that expect a problem, when no problem has been loaded * When the TSP importer is used after halting the execution, the engine should be reset * `RealVectorEncoding`: * Looks like we need a !SelfAdaptiveCrossover * `Optimization`: * Interface for selfadaptive crossover and selfadaptive mutator * `EvolutionStrategy`: * Prevent self adaptive mutation with non-selfadaptive crossover * Add parameter to ES to indicate if 1/5 success rule or sigma-self adapation should be used * !BoundsChecking after Crossover and Mutation with optional attribute * Implement `SelfAdaptiveCrossover` on the algorithm side with two `ICrossover` parameters === Priority: MEDIUM === * Saving a problem doesn't remember the filename when saving again * The TSP problem view: * Would be nice to have paste support (e.g. from Excel) in the datagrid of the string convertible matrix view. * 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) * In the operator graph chart view: * The vertical scroll bar allows scrolling way past the graph. * abeham: On second review, this isn't actually an issue. * 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. * 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. * !OpenView should be default double click behavior in operator graph chart. * mkommend: implemented with r3176. * Sometimes arrows cannot be selected (e.g. when they're pointing directly east). * mkommend: corrected selection of connections with r3175. * Some boxes are sometimes placed above the top end of the chart * mkommend: This behavior could not be reproduced in r3176. * Operators with a breakpoint set should be in some way highlighted * abeham: this actually works, maybe there's some case where this doesn't work? * A selected box should change the mouse cursor to the move cursor when hovering over it. * mkommend: implemented with r3171. * Just single clicking the successor port with the connector tool connects the box with itself, I don't think this is a desired behavior. * I would be nice if: As long the space keyboard is pressed the selection mode switches to panning * mkommend: implemented with r3173. * 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. * 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. * Viewing different result variables (switching between them) is impossible, because the results view is disabled when running the engine. * `TypeSelector` view: When only one single type is possible (e.g. `DoubleData`), select that type and auto-close with ok * A problem should inject a variable that indicates its dimensionality (sometimes you want to set values depending on the dimension of the problem) === Priority: LOW === * 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. * There should be a nicer way of saying the chart controls are not installed instead of an exception. * swagner: The HeuristicLab setup will check if the Microsoft Chart Controls are installed and will show a meaningful error message. * Does it make sense to select no engine in an user defined algorithm? * The button for changing the evaluation in the TSP view features "+" which is used for adding, not setting in other views. * swagner: In a discussion with maffenze it was decided to use the "+" icon for adding and for setting values as long as there is no better idea which icon to use for setting values. * Does it make sense to change the name of result variables or to remove their values? * 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 functionality (would be great to be able to specify resolution and width/height). * The tooltips should not exceed a certain width. With a long description you've got one line running over the whole screen. * swagner: It is not possible to define a maximum text width and to enforce word wrapping for tool tips. Therefore line breaks have to be set for each tool tip text individually. Please specify which tool tips are too long so that appropriate line breaks can be added. * The `DataTableValuesCollector` says it can only display double values, why not ints? * 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). * Going through the scopes with the arrow keys doesn't update the details view of the scope. * Resetting the engine doesn't update the global scope details view (user defined algorithm). * `TypeSelector` view: Focus should be in the search field when opening it the first time * abeham: Actually I like it on the ok button, but also because it's a fast way of auto-oking those single option cases (see above) * `VariableCreator` throws an exception when injecting a variable whose value is null * 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. * Improve description of !SearchIntervalFactor in `BreederGeneticAlgorithmManipulator` ---- == Reviewer: gkronber == === Priority: HIGH === * The plugin management GUI is not finished. === Priority: MEDIUM === * `HeuristicLab.Algorithms.TS` and `HeuristicLab.Algorithms.LS` should be renamed to `HeuristicLab.Algorithms.TabuSearch` and `HeuristicLab.Algorithms.LocalSearch`. * `HeuristicLab.Algorithms.SGA` should be renamed to `HeuristicLab.Algorithms.StandardGeneticAlgorithm` * Breakpoints should be visually emphasized. * swagner: At the moment, breakpoints are marked red in the `OperatorTreeView` and will be visually emphasized in the `OperatorGraph` visualization in the next version. Where else should they be emphasized? * 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 === * `ProgrammableOperator`: 'Compile' is a normal button but 'Show Generated Code' looks like a label. I think the style should be unified. ---- == Reviewer: swinkler == === Priority: HIGH === * Adding a paramter to a `ProgrammableOperator`, for example a `ValueParameter`, 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 a new SGA and loading a problem the "play" button is enabled; clicking it leads to a error report that might be not interpretable by each potential user. A rather complicated check of all parameters would be needed here (as we had it for example in HL 2) - this might be not essentially necessary for the initial release (?), but maybe we could keep it in mind. * swagner: Changed in r2924 that a selection and a crossover operator are automatically selected if available to avoid the cryptic error message. In general I would prefer to set reasonable parameters by default instead of implementing some (probably quite sophisticated) parameter validation procedure. === Priority: LOW === * 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.). * 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. ---- == Reviewer: maffenze == === Priority: HIGH === * 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. === Priority: LOW === ---- == Reviewer: vdorfer == 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. === Priority: HIGH === === Priority: MEDIUM === * 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. * 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 klick it, or else I have to start all over again * Add != Set Value; In all forms with a simple value (`IntData`, `DoubleData`, `BoolData`,..) a plus icon signalizes the possible change/edit. I would change the plus sign for example to a pencil (as you do not add a new value but change it). * A "Do you want to save your work" question is missing when clicking the "x" of an open SGA, TSP,... === Priority: LOW === * The comments in the generated code of a `ProgrammableOperator` are German. ---- == Reviewer: mkommend == === Priority: HIGH === === Priority: MEDIUM === * In my opinion a reordering of the tabs in the SGAView should be considered. The current order is Parameters, Problem, Engine, Results. It would be better if the order reflects the workflow to create an engine, e.g. Problem, Parameters, Results, Engine. === Priority: LOW === * The new dialog should also react on double clicks on the description column, if this is possible. ----