[GitHub] [flink] becketqin commented on a change in pull request #8632: [FLINK-12744][ml] add shared params in ml package

[GitBox <gi...@apache.org>]

becketqin commented on a change in pull request #8632: [FLINK-12744][ml] add shared params in ml package
URL: https://github.com/apache/flink/pull/8632#discussion_r292840459
 
 

 ##########
 File path: flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java
 ##########
 @@ -47,14 +70,33 @@
 	 * @throws RuntimeException if the Params doesn't contains the specific parameter, while the
 	 *                          param is not optional but has no default value in the {@code info}
 	 */
-	@SuppressWarnings("unchecked")
 	public <V> V get(ParamInfo<V> info) {
-		V value = (V) paramMap.getOrDefault(info.getName(), info.getDefaultValue());
-		if (value == null && !info.isOptional() && !info.hasDefaultValue()) {
-			throw new RuntimeException(info.getName() +
-				" not exist which is not optional and don't have a default value");
+		Stream<V> stream = getParamNameAndAlias(info)
+			.filter(this.params::containsKey)
+			.map(x -> this.params.get(x))
+			.map(x -> valueFromJson(x, info.getValueClass()))
+			.limit(1);
 
 Review comment:
   I am actually wondering if the behavior here is what we really want. If user specified values for multiple alias, shouldn't we throw exception here?
   
   Regarding the code style. I am fine either way. Readability wise, I'd rather not use stream here. Not using Stream does not necessarily result in more code. The following code snippet has same number of lines of code compared with the current PR. But it handles more cases and provides a pretty decent error message and comments to the users. Even without the comments I think it is still easier to understand.
   
   ```
   		String value = null;
   		String usedParamName = null;
   		for (String nameOrAlias : getParamNameAndAlias(info)) {
   			String v = params.get(nameOrAlias);
   			if (value != null && v != null) {
   				throw new IllegalArgumentException(String.format("Duplicate parameters of %s and %s",
   						usedParamName, nameOrAlias));
   			} else if (v != null) {
   				value = v;
   				usedParamName = nameOrAlias;
   			}
   		}
   
   		if (value != null) {
   			// The param value was set by the user.
   			return valueFromJson(value, info.getValueClass());
   		} else {
   			// The param value was not set by the user.
   			if (!info.isOptional()) {
   				throw new IllegalArgumentException("Missing non-optional parameter " + info.getName());
   			} else if (!info.hasDefaultValue()) {
   				throw new IllegalArgumentException("Cannot find default value for optional parameter " + info.getName());
   			}
   			return info.getDefaultValue();
   		}
   ```

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services