[GitHub] [flink] becketqin commented on a change in pull request #8776: [FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class

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

becketqin commented on a change in pull request #8776: [FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class
URL: https://github.com/apache/flink/pull/8776#discussion_r296509197
 
 

 ##########
 File path: flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/ParamInfo.java
 ##########
 @@ -19,12 +19,25 @@
 package org.apache.flink.ml.api.misc.param;
 
 import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.util.Preconditions;
 
 /**
  * Definition of a parameter, including name, type, default value, validator and so on.
  *
  * <p>This class is provided to unify the interaction with parameters.
  *
+ * <p>when isOptional is true, contain(ParamInfo) is true, it will return the value found in Params,
 
 Review comment:
   This class is only about definition. It might be better to keep the semantic of `contains()` and `get()` in there own method java doc. How about just mention the following here? Also, should we throw exception if a default value is specified for a non-optional parameter?
   
   ```
   /**
    * Definition of a parameter, including name, type, default value, validator and so on.
    *
    * <p>A parameter can either be optional or non-optional.
    * <ul>
    *     <li>
    *         A non-optional parameter should not have a default value. Instead, its value must be provided by the users.
    *     </li>
    *     <li>
    *         An optional parameter may or may not have a default value.
    *     </li>
    * </ul>
    *
    * <p>Please see {@link Params#get(ParamInfo)} and {@link Params#contains(ParamInfo)} for more details about the behavior.
    *
    * <p>A parameter may have aliases in addition to the parameter name for convenience and compatibility purposes. One
    * should not set values for both parameter name and an alias. One and only one value should be set either under
    * the parameter name or one of the alias.
    *
    * @param <V> the type of the param value
    */
   ```

----------------------------------------------------------------
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