cpoerschke commented on a change in pull request #2350:
URL: https://github.com/apache/lucene-solr/pull/2350#discussion_r575388380
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/model/LinearModel.java
##########
@@ -80,10 +80,10 @@
public void setWeights(Object weights) {
@SuppressWarnings({"unchecked"})
- final Map<String,Double> modelWeights = (Map<String,Double>) weights;
+ final Map<String,Number> modelWeights = (Map<String, Number>) weights;
for (int ii = 0; ii < features.size(); ++ii) {
final String key = features.get(ii).getName();
- final Double val = modelWeights.get(key);
+ final Number val = modelWeights.get(key);
Review comment:
neat!
##########
File path:
solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedModelStore.java
##########
@@ -294,11 +294,18 @@ private static void initWrapperModel(SolrResourceLoader
solrResourceLoader,
return modelMap;
}
- private static Feature lookupFeatureFromFeatureMap(Map<String,Object>
featureMap,
- FeatureStore featureStore) {
- final String featureName = (String)featureMap.get(NAME_KEY);
- return (featureName == null ? null
- : featureStore.get(featureName));
+ private static Feature lookupFeatureFromFeatureMap(Map<String, Object>
featureMap, FeatureStore featureStore)
+ {
+ final String featureName = (String) featureMap.get(NAME_KEY);
+ Feature extractedFromStore = featureName == null ? null :
featureStore.get(featureName);
+ if (extractedFromStore == null) {
+ if (featureStore.getFeatures().isEmpty()) {
+ throw new ModelException("Feature Store not found: " +
featureStore.getName());
+ } else {
+ throw new ModelException("Feature:" + featureName + " not found in
store: " + featureStore.getName());
Review comment:
```suggestion
throw new ModelException("Feature: " + featureName + " not found in
store: " + featureStore.getName());
```
##########
File path:
solr/contrib/ltr/src/java/org/apache/solr/ltr/model/LTRScoringModel.java
##########
@@ -108,7 +108,7 @@ public static LTRScoringModel
getInstance(SolrResourceLoader solrResourceLoader,
SolrPluginUtils.invokeSetters(model, params.entrySet());
}
} catch (final Exception e) {
- throw new ModelException("Model type does not exist " + className, e);
+ throw new ModelException("Model creation failed for " + className, e);
Review comment:
minor: how about not saying "creation" but perhaps "loading" or
something to that effect? technically the "creation" i.e. storage of the
model's JSON succeeded (if i remember things right) but it's the "loading" or
"using" of the model's JSON that encounters the error. so the user seeing the
error needs to look at the content of what they uploaded but not at the
mechanics of how the uploaded/created the model.
##########
File path:
solr/contrib/ltr/src/test-files/modelExamples/multipleadditivetreesmodel_unknownFeature.json
##########
@@ -0,0 +1,38 @@
+{
+ "class":"org.apache.solr.ltr.model.MultipleAdditiveTreesModel",
+ "name":"multipleadditivetreesmodel",
+ "features":[
+ { "name": "notExist1"},
+ { "name": "notExist2"}
+ ],
+ "params":{
+ "trees": [
+ {
+ "weight" : "1f",
+ "root": {
+ "feature": "matchedTitle",
+ "threshold": "0.5f",
+ "left" : {
+ "value" : "-100"
+ },
+ "right": {
+ "feature" :
"constantScoreToForceMultipleAdditiveTreesScoreAllDocs",
Review comment:
```suggestion
"feature" : "notExist2",
```
##########
File path:
solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedModelStore.java
##########
@@ -294,11 +294,18 @@ private static void initWrapperModel(SolrResourceLoader
solrResourceLoader,
return modelMap;
}
- private static Feature lookupFeatureFromFeatureMap(Map<String,Object>
featureMap,
- FeatureStore featureStore) {
- final String featureName = (String)featureMap.get(NAME_KEY);
- return (featureName == null ? null
- : featureStore.get(featureName));
+ private static Feature lookupFeatureFromFeatureMap(Map<String, Object>
featureMap, FeatureStore featureStore)
+ {
+ final String featureName = (String) featureMap.get(NAME_KEY);
+ Feature extractedFromStore = featureName == null ? null :
featureStore.get(featureName);
+ if (extractedFromStore == null) {
+ if (featureStore.getFeatures().isEmpty()) {
+ throw new ModelException("Feature Store not found: " +
featureStore.getName());
Review comment:
```suggestion
throw new ModelException("Missing or empty feature store: " +
featureStore.getName());
```
##########
File path:
solr/contrib/ltr/src/test-files/modelExamples/multipleadditivetreesmodel_unknownFeature.json
##########
@@ -0,0 +1,38 @@
+{
+ "class":"org.apache.solr.ltr.model.MultipleAdditiveTreesModel",
+ "name":"multipleadditivetreesmodel",
+ "features":[
+ { "name": "notExist1"},
+ { "name": "notExist2"}
+ ],
+ "params":{
+ "trees": [
+ {
+ "weight" : "1f",
+ "root": {
+ "feature": "matchedTitle",
Review comment:
```suggestion
"feature": "notExist1",
```
##########
File path:
solr/contrib/ltr/src/test-files/modelExamples/linear-model_notExistentFeature.json
##########
@@ -0,0 +1,24 @@
+{
+ "class": "org.apache.solr.ltr.model.LinearModel",
+ "name": "6029760550880411648",
+ "store": "test",
+ "features": [
+ {
+ "name": "notExist1"
+ },
+ {
+ "name": "notExist2"
+ }
+ ],
+ "params": {
+ "weights": {
+ "title": 0.0000000000,
+ "description": 0.1000000000,
+ "keywords": 0.2000000000,
+ "popularity": 0.3000000000,
+ "text": 0.4000000000,
+ "queryIntentPerson": 0.1231231,
+ "queryIntentCompany": 0.12121211
Review comment:
how about using `"notExist1"` and `"notExist2"` here for the weights too?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
