lindong28 commented on a change in pull request #35:
URL: https://github.com/apache/flink-ml/pull/35#discussion_r749482240
##########
File path: flink-ml-api/src/main/java/org/apache/flink/ml/param/WithParams.java
##########
@@ -72,6 +72,14 @@
*/
@SuppressWarnings("unchecked")
default <V> T set(Param<V> param, V value) {
+ if (!getParamMap().containsKey(param)) {
Review comment:
Yeah I think `set()` is consistent with `get()`.
The code is the `get(...)` is expected to use `&&`. Here is why:
- If `value == null` and `param.validator.validate(value) == true`, we want
`get(...)` to return null without throwing exception. This is needed in the
scenario where we define a parameter with defaultValue=null and
validator=`alwaysTrue()`.
- If `value == null` and `param.validator.validate(value) == false`, the
`null` value must come from the default parameter value. It means that user has
required a non-null value, set default value = null, and called `get(...)`
without setting a non-null value. In this case we want to throw exception.
- If `value != null`, the non-null value can come from either the default
value, or from the `set(...)` call. In the former case, the Param constructor
would have already verified that `param.validator.validate(value) == true`. In
the latter case, `set(...)` would have verified that
`param.validator.validate(value) == true`. Thus there is no need to
additionally check `param.validator.validate(value)` in this case. And we don't
need to throw exception here.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
