LabreucheModel

[oliviercailloux]

• if call multiple times isApplicable(Anchor) or getSomethingExplanation(), should compute only once. Document (private javadoc) that the variable is null iff it has not been initialized.
• remove phiALL and other output variables, use only labreucheOutput and cast as necessary.
• accept AlternativesComparison instead in LabreucheModel(), remove redundant three variables.
• epsilon must be given in parameter (or even better: think about a clever default choice (should depend on the weights), and propose two constructors)
• remove redundant isSomethingApplicable()
• methods do one thing and are named accordingly, for example showProblem() if you want to show the problem (instead of startproblem), and are located in appropriate class (for example showProblem() could be in AlternativesComparison I suppose)
• replace getWeight(), getX(), getY() by getAlternativesComparison() (simplifies interface).

[tonyseg]

Did I miss something to do here? I did'nt modified the value of epsilon because I use the method given by Labreuche in his paper.

[oliviercailloux]

• Try to reduce number of methods: remove redundant ones or generalize (displayAsVector and showVector do almost the same thing; showGraph is probably not useful); move general utilities (not Labreuche-related) to a more general Utils class (for example showVector).
• Clarify input parameters, only require what is really used (example showVector only uses the values of the map so the method doesn’t need a map as input, and by the way, it should use a number formatter internally)
• Is List of criteria (instead of Set) really needed? If not, remove all those methods to simplify code.
• Replace collections of Couple by graphs everywhere possible.
• showProblem() does not really seem to belong to AlternativesComparison, as it is currently defined.
• LabreucheModel should have at least one method whose name clearly indicates that it computes the solution. This method should probably be decomposed into several methods to avoid very long methods. Currently isApplicable does the job but its name does not indicate it; and it is much too long.
• in getSomethingExplanation, must preconditions as soon as possible, not at the end of the method (it’s a general rule to be applied everywhere possible). It should be visible where it computes, if it does (cf. previous point).
• add tests for more Labreuche results as in the report.
• move argue logic out of Model, as discussed
• decide whether you consider weights should sum to one and document and modify the code accordingly

[oliviercailloux]

Excellent improvement!

Small details:

• iterate only on the information you need for more clarity, example in tryALL, iterating over alternativesComparison.getDelta().values() is enough.
• after call to compute, assert that labreucheOutput is not null (to make clear and make sure that null is never returned).
• in isApplicable, return labreucheOutput.getAnchor() == anchor.
• getCheckedExplanation, not getCheckExplanation.

Replace more “couple” by graphs where it’s easy: couples_of, tryIVT.

[tonyseg]

Small details fixed and List<Couple<Criterion,Criterion> turned into Graph.

[oliviercailloux]

Very good job about Graphs. Still a few (very small) details:

• write assert labreucheOutput != null; instead of assert !(labreucheOutput == null);
• add preconditions check that labreucheOutput is null in the try* methods
• the javadoc on computeExplanation() is incorrectly formatted.

I believe the test in includeDiscri if (d_eu(a.get(k), w, delta) == d_eu(b.get(k), w, delta)) can never be true. (How come the code works?!)

You can probably simplify d_eu as you seem to only use the “left” value (except in the place above, is it important?).

[tonyseg]

Small details fixed.

Indeed, in includeDisicri it can be true only if the Couples returned are the same. SImply need to add .getLeft(). Fortunately it was never trigerred this condition in the examples.

for d_eu : in tryIVT I use getRight() on the Couple returned in order to avoid computing the minimal permutation again.

[oliviercailloux]

Very good improvement overall. I’ll open more issues for the remaining points.