Add getBestMove and FirstBestAdmissibleTabuSearch #39

huanfachen · 2017-05-31T10:25:59Z

Add a new getBestMove to NeighbourhoodSearch() with a parameter boolean acceptFirstImprovement.
Add FirstBestAdmissibleTabuSearch.java

…arch.java

coveralls · 2017-05-31T10:32:47Z

Coverage decreased (-2.08%) to 95.095% when pulling 1c9e3b8 on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:master.

…AdmissibleTabuSearch

coveralls · 2017-05-31T12:20:03Z

Coverage decreased (-2.1%) to 95.095% when pulling e06489b on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

hdbeukel

I have added some comments. Can you have a look? Please also add tests to get the coverage back up.

hdbeukel · 2017-05-31T12:07:05Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+        // track first improvement move AND best valid move + corresponding evaluation, validation and delta
+        Move<? super SolutionType> bestMove = null, firstImprovementMove = null;
+        double bestMoveDelta = -Double.MAX_VALUE, curMoveDelta, firstImprovementMoveDelta = -Double.MAX_VALUE;
+        Evaluation curMoveEvaluation, bestMoveEvaluation = null, firstImprovementMoveEvaluation = null;


You don't need separate variables for first and best improvement. One variable, say chosenMove, is sufficient to store the move you selected (either first improvement, or best). Then you also have chosenMoveDelta, chosenMoveEvaluation, and chosenMoveValidation.

hdbeukel · 2017-05-31T12:08:14Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+    @SafeVarargs
+    protected final Move<? super SolutionType> getBestMove(List<? extends Move<? super SolutionType>> moves,
+                                                           boolean requireImprovement,
+                                                           boolean acceptFirstImprovement,


Parameter acceptFirstImprovement is never used. You implementation returns the first improvement, even if acceptFirstImprovement is false. See comments below to fix.

hdbeukel · 2017-05-31T12:11:50Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+        Evaluation curMoveEvaluation, bestMoveEvaluation = null, firstImprovementMoveEvaluation = null;
+        Validation curMoveValidation, bestMoveValidation = null, firstImprovementMoveValidation = null;
+        // go through all moves
+        for (Move<? super SolutionType> move : moves) {


Please use an Iterator here in a while loop with multiple conditions to avoid the use of break. You should continue as long as there are more moves to try and !(acceptFirstImprovement && chosenMoveDelta > 0).

hdbeukel · 2017-05-31T12:24:17Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+     * @return first improving move or the best valid move, may be <code>null</code>
+     */
+    @SafeVarargs
+    protected final Move<? super SolutionType> getBestMove(Collection<? extends Move<? super SolutionType>> moves,


Once this method's implementation is finished, the other getBestMove method should relay to this one with acceptFirstImprovement set to false.

Agree with the suggestion. Before adding tests, I found a problem in Line 445 of GenericProblem.java, which prevents the running of NeighbourhoodSearchTest. Could you have a look at it?

It's not a problem with JAMES but with Eclipse. See #41 and #31.

Problem solved by switching to IntelliJ IDEA

hdbeukel · 2017-05-31T13:17:52Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+    @SafeVarargs
+    protected final Move<? super SolutionType> getBestMove(Collection<? extends Move<? super SolutionType>> moves,
+                                                           boolean requireImprovement,
+                                                           boolean acceptFirstImprovement,


the parameter acceptFirstImprovement is never checked.

hdbeukel · 2017-05-31T13:18:52Z

src/main/java/org/jamesframework/core/search/NeighbourhoodSearch.java

+        // track first improvement move AND best valid move + corresponding evaluation, validation and delta
+        Move<? super SolutionType> bestMove = null, firstImprovementMove = null;
+        double bestMoveDelta = -Double.MAX_VALUE, curMoveDelta, firstImprovementMoveDelta = -Double.MAX_VALUE;
+        Evaluation curMoveEvaluation, bestMoveEvaluation = null, firstImprovementMoveEvaluation = null;


No need to store first and best move (and their metadata separately). You can have one series of variables for the chosen move, say chosenMove, chosenMoveDelta, etc. which will either be the first or best one depending on the parameter acceptFirstImprovement.

…arch

coveralls · 2017-05-31T17:40:46Z

Coverage decreased (-0.7%) to 96.555% when pulling a25b183 on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

coveralls · 2017-06-01T09:59:19Z

Coverage decreased (-0.7%) to 96.555% when pulling df1fe55 on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

hdbeukel

Nice work. I added some more suggestions.

hdbeukel · 2017-06-01T12:36:46Z

pom.xml

@@ -7,6 +7,7 @@
        <groupId>org.jamesframework</groupId>
        <artifactId>james</artifactId>
        <version>1.3-SNAPSHOT</version>
+        <relativePath>../james/james</relativePath>


Why has this line been added? Should not be necessary.

It is added to make the codes work on my computer, as I cloned James and James-core separately. I have added pom.xml to .gitignore. Please revise the pom.xml file accordingly.

I see. I will remove this line when merging the pull request.

hdbeukel · 2017-06-01T12:40:22Z

src/main/java/org/jamesframework/core/search/algo/tabu/FirstBestAdmissibleTabuSearch.java

+ * @param <SolutionType> solution type of the problems that may be solved using this search, required to extend {@link Solution}
+ * @author <a href="mailto:chenhuanfa@gmail.com">Huanfa Chen</a>
+ */
+public class FirstBestAdmissibleTabuSearch<SolutionType extends Solution> extends TabuSearch<SolutionType>{


Please also test this new Tabu search.

Test added in the new pull request.

Please don't create a new pull request for each commit. Just keep committing to the same branch, all changes will show up here. I closed the other pull request #43.

I am not very familiar with Github. Say, I add and commit the changes, and push them to the remote repo. Will it be automatically committed to the same branch?

If you don't specify an explicit branch when pushing, git will push to the remote branch that has been associated with your local branch. So yes, these commits will end up in the same branch each time. In your case tabuSearch/add_variant.

Are you using git on command line or a desktop client?

I am using git bash on Windows. Just add a new push.

hdbeukel · 2017-06-01T12:42:55Z

src/test/java/org/jamesframework/core/search/NeighbourhoodSearchTest.java

-
+
+    @Test
+    public void testGetBestMoveAcceptFirstImprovement() throws Exception {


Please move test method down (alongside other test methods instead of in between the setup code).

hdbeukel · 2017-06-01T12:49:11Z

src/test/java/org/jamesframework/core/search/NeighbourhoodSearchTest.java

+        assertTrue(bestMove != null);
+        assertTrue(bestMoveFirstImprovement != null);
+        assertTrue(bestMove == bestMoveFirstImprovement);
+        assertTrue(neighSearch.evaluate(bestMove).getValue() == neighSearch.evaluate(bestMoveFirstImprovement).getValue());


Please avoid comparing double values with ==. Either use assertEquals(double expected, double actual, double delta) or just remove this line because it can never fail if the previous line succeeds.

hdbeukel · 2017-06-01T12:54:06Z

src/test/java/org/jamesframework/core/search/NeighbourhoodSearchTest.java

+            bestMoveFirstImprovement = neighSearch.getBestMove(moves, true,true);
+        }
+
+        // when curSolution is best, bestMove and bestMoveFirstImprovement should be the (nearly) the same


Nice test case! Why nearly the same? They should be exactly the same no? (which is what you test)

Could you perhaps think of a way to also explicitly test the other case, i.e. when the first and best improvement are not the same?

hdbeukel · 2017-06-01T12:57:23Z

Please remove the following files and folders (IntelliJ/Eclipse settings) and include them in .gitignore to avoid that they are added in the future:

.classpath
.project
.settings
james-core.iml (add *.iml to .gitignore)
.idea

Also see my other comments.

hdbeukel · 2017-06-02T11:38:27Z

You have several compiler warnings in your code. See https://travis-ci.org/hdbeukel/james-core/builds/238702467?utm_source=github_status&utm_medium=notification. Please fix.

hdbeukel · 2017-06-02T11:38:41Z

The undesired files listed above are also still present.

coveralls · 2017-06-02T12:39:23Z

Coverage decreased (-0.08%) to 97.146% when pulling 5d564cb on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

coveralls · 2017-06-02T12:49:33Z

Coverage decreased (-0.1%) to 97.096% when pulling 9eaf5c4 on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

hdbeukel · 2017-06-02T13:40:58Z

Tests fail now because you have deleted the file pom.xml (alongside the undesired files).

coveralls · 2017-06-05T09:28:26Z

Coverage decreased (-0.08%) to 97.146% when pulling 37e899e on huanfachen:tabuSearch/add_variant into f6bfd50 on hdbeukel:tabuvariant.

hdbeukel · 2017-06-05T10:33:13Z

Thanks for the fix. I will get back to this next week and merge your pull request (no time this week).

hdbeukel · 2017-06-05T10:34:28Z

I will probably make some more changes (e.g. Javadocs). If you want you can then pull them back into your fork. I will send you a pull request.

This reverts commit e4db65a.

Add getBestMove to NeighbourhoodSearch. Add FirstBestAdmissibleTabuSe…

1c9e3b8

…arch.java

hdbeukel changed the base branch from master to tabuvariant May 31, 2017 11:56

Revise getBestMove in NeighbourhoodSearch and searchStep in FirstBest…

e06489b

…AdmissibleTabuSearch

hdbeukel requested changes May 31, 2017

View reviewed changes

Revise TabuSearch, FirstBestAdmissibleTabuSearch, and NeighbourhoodSe…

a25b183

…arch

Revise getBestMove. Add testGetBestMoveAcceptImprovement.

df1fe55

hdbeukel requested changes Jun 1, 2017

View reviewed changes

Add test of FirstBestAdmissibleTabuSearch. Revise files.

80184ef

hdbeukel approved these changes Jun 2, 2017

View reviewed changes

Revise NeighbourhoodSearchTest

5d564cb

huanfachen added 2 commits June 2, 2017 13:42

Ignore undesirable files

9eaf5c4

use new .gitignore

d687bc9

use new .gitignore

37e899e

hdbeukel merged commit e4db65a into hdbeukel:tabuvariant Jun 14, 2017

hdbeukel added a commit that referenced this pull request Jun 14, 2017

Revert "Add FirstBestAdmissibleTabuSearch (#39)"

d96d7fc

This reverts commit e4db65a.



		@Test
		public void testGetBestMoveAcceptFirstImprovement() throws Exception {

Add getBestMove and FirstBestAdmissibleTabuSearch #39

Add getBestMove and FirstBestAdmissibleTabuSearch #39

Conversation

huanfachen commented May 31, 2017

coveralls commented May 31, 2017

coveralls commented May 31, 2017

hdbeukel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 31, 2017

coveralls commented Jun 1, 2017

hdbeukel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdbeukel Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdbeukel Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

hdbeukel commented Jun 1, 2017

hdbeukel commented Jun 2, 2017

hdbeukel commented Jun 2, 2017

coveralls commented Jun 2, 2017

coveralls commented Jun 2, 2017

hdbeukel commented Jun 2, 2017

coveralls commented Jun 5, 2017

hdbeukel commented Jun 5, 2017

hdbeukel commented Jun 5, 2017

hdbeukel left a comment •

edited

Loading

hdbeukel Jun 2, 2017 •

edited

Loading

hdbeukel Jun 1, 2017 •

edited

Loading