From c8d3098d3e8531e76ed584568aa70673da0bb2d2 Mon Sep 17 00:00:00 2001
From: Isabel Drost-Fromm <isabel.drostfromm@elasticsearch.com>
Date: Thu, 21 Jul 2016 15:44:47 +0200
Subject: [PATCH] Move github review comments to TODOs

---
 .../java/org/elasticsearch/index/rankeval/PrecisionAtN.java   | 1 +
 .../org/elasticsearch/index/rankeval/RankEvalResponse.java    | 1 +
 .../java/org/elasticsearch/index/rankeval/RankEvalResult.java | 1 +
 .../elasticsearch/index/rankeval/RankedListQualityMetric.java | 1 +
 .../java/org/elasticsearch/index/rankeval/RatedDocument.java  | 1 +
 .../org/elasticsearch/index/rankeval/RestRankEvalAction.java  | 1 +
 .../elasticsearch/index/rankeval/TransportRankEvalAction.java | 1 +
 .../java/org/elasticsearch/index/rankeval/QuerySpecTests.java | 4 +++-
 8 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/PrecisionAtN.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/PrecisionAtN.java
index 1d67c45dbb8..11101826ecb 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/PrecisionAtN.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/PrecisionAtN.java
@@ -128,6 +128,7 @@ public class PrecisionAtN extends RankedListQualityMetric {
         return new EvalQueryQuality(precision, unknownDocIds);
     }
 
+    // TODO add abstraction that also works for other metrics
     public enum Rating {
         IRRELEVANT, RELEVANT;
     }
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResponse.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResponse.java
index 7af60c24151..0e520febdb8 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResponse.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResponse.java
@@ -38,6 +38,7 @@ import java.util.Map;
  * Documents of unknown quality - i.e. those that haven't been supplied in the set of annotated documents but have been returned
  * by the search are not taken into consideration when computing precision at n - they are ignored.
  *
+ * TODO get rid of either this or RankEvalResult
  **/
 public class RankEvalResponse extends ActionResponse implements ToXContent {
 
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResult.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResult.java
index 09c55b32b25..726b3c82aa7 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResult.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalResult.java
@@ -32,6 +32,7 @@ import java.util.Map;
  * for reference. In addition the averaged precision and the ids of all documents returned but not found annotated is returned.
  * */
 // TODO do we need an extra class for this or it RankEvalResponse enough?
+// TODO instead of just returning averages over complete results, think of other statistics, micro avg, macro avg, partial results
 public class RankEvalResult implements Writeable {
     /**ID of QA specification this result was generated for.*/
     private String specId;
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankedListQualityMetric.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankedListQualityMetric.java
index 829be8a9bc5..76c4526ec0d 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankedListQualityMetric.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankedListQualityMetric.java
@@ -54,6 +54,7 @@ public abstract class RankedListQualityMetric implements NamedWriteable {
         }
         String metricName = parser.currentName();
 
+        // TODO maybe switch to using a plugable registry later?
         switch (metricName) {
         case PrecisionAtN.NAME:
             rc = PrecisionAtN.fromXContent(parser, context);
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedDocument.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedDocument.java
index d1bd99b97c4..8915a309366 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedDocument.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedDocument.java
@@ -33,6 +33,7 @@ import java.io.IOException;
  * */
 public class RatedDocument implements Writeable {
 
+    // TODO augment with index name and type name
     private final String docId;
     private final int rating;
 
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RestRankEvalAction.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RestRankEvalAction.java
index 32f2438449a..5034e8350a2 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RestRankEvalAction.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RestRankEvalAction.java
@@ -192,6 +192,7 @@ public class RestRankEvalAction extends BaseRestHandler {
             QueryParseContext parseContext = new QueryParseContext(queryRegistry, parser, parseFieldMatcher);
             if (restContent != null) {
                 parseRankEvalRequest(rankEvalRequest, request,
+                        // TODO can we get rid of aggregators parsers and suggesters?
                         new RankEvalContext(parseFieldMatcher, parseContext, aggregators, suggesters));
             }
         }
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java
index 47110412d42..2dbc44d7141 100644
--- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java
+++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java
@@ -103,6 +103,7 @@ public class TransportRankEvalAction extends HandledTransportAction<RankEvalRequ
             unknownDocs.put(spec.getSpecId(), intentQuality.getUnknownDocs());
         }
         RankEvalResponse response = new RankEvalResponse();
+        // TODO move averaging to actual metric, also add other statistics
         RankEvalResult result = new RankEvalResult(qualityTask.getTaskId(), qualitySum / specifications.size(), unknownDocs);
         response.setRankEvalResult(result);
         listener.onResponse(response);
diff --git a/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/QuerySpecTests.java b/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/QuerySpecTests.java
index 49ae4d0d6e4..1f1814163e3 100644
--- a/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/QuerySpecTests.java
+++ b/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/QuerySpecTests.java
@@ -35,6 +35,7 @@ import org.junit.AfterClass;
 import org.junit.BeforeClass;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.List;
 
 public class QuerySpecTests extends ESTestCase {
@@ -50,7 +51,7 @@ public class QuerySpecTests extends ESTestCase {
     @BeforeClass
     public static void init() throws IOException {
         aggsParsers = new AggregatorParsers(new ParseFieldRegistry<>("aggregation"), new ParseFieldRegistry<>("aggregation_pipes"));
-        searchModule = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry(), false);
+        searchModule = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry(), false, new ArrayList<>());
         queriesRegistry = searchModule.getQueryParserRegistry();
         suggesters = searchModule.getSuggesters();
     }
@@ -63,6 +64,7 @@ public class QuerySpecTests extends ESTestCase {
         aggsParsers = null;
     }
 
+    // TODO add some sort of roundtrip testing like we have now for queries?
     public void testParseFromXContent() throws IOException {
         String querySpecString = " {\n"
          + "   \"id\": \"my_qa_query\",\n"