cpoerschke commented on a change in pull request #123:
URL: https://github.com/apache/solr/pull/123#discussion_r633135019
##########
File path:
solr/contrib/ltr/src/test/org/apache/solr/ltr/feature/TestFieldValueFeature.java
##########
@@ -56,11 +62,13 @@ public void before() throws Exception {
assertU(commit());
- loadFeature("popularity", FieldValueFeature.class.getName(),
- "{\"field\":\"popularity\"}");
+ for (String field : FIELD_NAMES) {
+ loadFeature(field, FieldValueFeature.class.getName(),
+ "{\"field\":\""+field+"\"}");
- loadModel("popularity-model", LinearModel.class.getName(),
- new String[] {"popularity"}, "{\"weights\":{\"popularity\":1.0}}");
+ loadModel(field + "-model", LinearModel.class.getName(),
+ new String[] {field}, "{\"weights\":{\""+field+"\":1.0}}");
+ }
Review comment:
2/2 ... I think the explanation for the mystery is here i.e. the
`LinearModel` constructor signature and the interleaving of feature and model
loading results in the models having different definitions of what
`allFeatures` means. One solution then would be for the test expectations to
account for the multiple meanings of `allFeatures` or (my preference probably)
for there to be only one `allFeatures` meaning e.g. via the separation of
feature and model loading. Or perhaps you can think of a third solution even?
```
for (String field : FIELD_NAMES) {
loadFeature(field, ...);
}
for (String field : FIELD_NAMES) {
loadModel(field + "-model", ...);
}
```
##########
File path:
solr/contrib/ltr/src/test/org/apache/solr/ltr/feature/TestFieldValueFeature.java
##########
@@ -70,86 +78,92 @@ public void after() throws Exception {
@Test
public void testRanking() throws Exception {
-
- final SolrQuery query = new SolrQuery();
- query.setQuery("title:w1");
- query.add("fl", "*, score");
- query.add("rows", "4");
-
- // Normal term match
- assertJQ("/query" + query.toQueryString(), "/response/numFound/==4");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='1'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='8'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='7'");
-
- query.add("rq", "{!ltr model=popularity-model reRankDocs=4}");
-
- assertJQ("/query" + query.toQueryString(), "/response/numFound/==4");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='8'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='7'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='1'");
-
- query.setQuery("*:*");
- query.remove("rows");
- query.add("rows", "8");
- query.remove("rq");
- query.add("rq", "{!ltr model=popularity-model reRankDocs=8}");
-
- assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='8'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='7'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='5'");
+ for (String field : FIELD_NAMES) {
+
+ final SolrQuery query = new SolrQuery();
+ query.setQuery("title:w1");
+ query.add("fl", "*, score");
+ query.add("rows", "4");
+
+ // Normal term match
+ assertJQ("/query" + query.toQueryString(), "/response/numFound/==4");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='1'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='8'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='7'");
+
+ query.add("rq", "{!ltr model="+field+"-model reRankDocs=4}");
+
+ assertJQ("/query" + query.toQueryString(), "/response/numFound/==4");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='8'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='7'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='1'");
+
+ query.setQuery("*:*");
+ query.remove("rows");
+ query.add("rows", "8");
+ query.remove("rq");
+ query.add("rq", "{!ltr model="+field+"-model reRankDocs=8}");
+
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='8'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[1]/id=='7'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[2]/id=='6'");
+ assertJQ("/query" + query.toQueryString(), "/response/docs/[3]/id=='5'");
+ }
}
@Test
public void testIfADocumentDoesntHaveAFieldDefaultValueIsReturned() throws
Exception {
- SolrQuery query = new SolrQuery();
- query.setQuery("id:42");
- query.add("fl", "*, score");
- query.add("rows", "4");
-
- assertJQ("/query" + query.toQueryString(), "/response/numFound/==1");
- assertJQ("/query" + query.toQueryString(), "/response/docs/[0]/id=='42'");
- query = new SolrQuery();
- query.setQuery("id:42");
- query.add("rq", "{!ltr model=popularity-model reRankDocs=4}");
- query.add("fl", "[fv]");
- assertJQ("/query" + query.toQueryString(), "/response/numFound/==1");
- assertJQ("/query" + query.toQueryString(),
-
"/response/docs/[0]/=={'[fv]':'"+FeatureLoggerTestUtils.toFeatureVector("popularity",Float.toString(FIELD_VALUE_FEATURE_DEFAULT_VAL))+"'}");
-
+ for (String field : FIELD_NAMES) {
+ SolrQuery query = new SolrQuery();
+ query.setQuery("id:42");
+ query.add("fl", "*, score");
+ query.add("rows", "4");
+
+ assertJQ("/query" + query.toQueryString(), "/response/numFound/==1");
+ assertJQ("/query" + query.toQueryString(),
"/response/docs/[0]/id=='42'");
+ query = new SolrQuery();
+ query.setQuery("id:42");
+ query.add("rq", "{!ltr model="+field+"-model reRankDocs=4}");
+ query.add("fl", "[fv]");
+ assertJQ("/query" + query.toQueryString(), "/response/numFound/==1");
+ assertJQ("/query" + query.toQueryString(),
+
"/response/docs/[0]/=={'[fv]':'"+FeatureLoggerTestUtils.toFeatureVector(field,Float.toString(FIELD_VALUE_FEATURE_DEFAULT_VAL))+"'}");
Review comment:
1/2 Thanks for investigating in detail here! I agree the explanation is
with the `extractAllFeatures` boolean i.e. the response contains the values for
all the features in the feature store. The puzzling thing then though is why
the first run of the loop succeeds and only subsequent runs fail. Since all
loop runs use the same feature store and surely each loop run should return the
same feature values? ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
