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:
[email protected]
With regards,
Apache Git Services
