Hi Jiangjie, Thanks for the review! Please find below my explanations.
On Fri, Oct 29, 2021 at 8:49 PM Becket Qin <becket....@gmail.com> wrote: > Hi Dong and Zhiping, > > Thanks for writing up the FLIP and sorry for the belated review. The FLIP > looks good to me overall. Just a few minor comments / questions about > WithParams. > > 1. Currently all the methods in the WithParams interface have a default > implementation. It is not clear to me what exactly these default > implementations will do. For example, in the example usage, MyParams > extends WithParams and has some static Param defined, how would the default > getParam() work in this case? > For those Param fields defined in the MyParams, WithParams::getParamMap(...) will return a map that contains values for those Param fields. getParam(...) will first call getParamMap(...) to get all parameter values of its instance and search for a Param that has the given parameter name. It returns the Param with the given name if it finds one, otherwise it returns null. > > 2. What is the difference between getParamMap() and > getUserDefinedParamMap()? It looks that one of them is mutable and the > other is not. Apart from that, are they going to return the same mapping? > If users want to set param values, can they just call set() directly? In > which case would they want to get a mutable param map and set the param > values directly in that map? > Both getParamMap(...) and getUserDefinedParamMap(...) contain values for parameters whose values have been explicitly set by WithParams::set(...). For those parameters whose values have not been explicitly set, getParamMap(...) will contain this parameter but getUserDefinedParamMap(...) will not. Ideally we should only need getParamMap(...) on this interface to get all parameters values. However, WithParams can not contain non-static member fields since it is an interface. Thus we need getUserDefinedParamMap(...) so that a concrete subclass of WithParams could override this method to provide a map object that can be used to store parameter values. To answer your questions: - These two maps could return different mapping. - Users can just call set() directly to set param values. A subclass of WithParams must override getUserDefinedParamMap(...) to return a map object. - Users will not need to directly call getUserDefinedParamMap(...) to get this mutable map. The default implementation of WithParams::set(...) and WithParams::get(...) will need to call getUserDefinedParamMap(...) to set/get parameter values. > Thanks, > > Jiangjie (Becket) Qin > > On Thu, Sep 23, 2021 at 1:33 PM Dong Lin <lindon...@gmail.com> wrote: > > > Hi all, > > > > Zhipeng and I have created FLIP-174: Improve the WithParam interface. > > Please find the FLIP wiki in the link > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=181311361 > > . > > > > The goal of this FLIP is to improve the experience of defining, saving > and > > loading parameters of stages (e.g. Transformer and Estimator) in the > Flink > > ML repo. > > > > It will be really helpful if you can help review this FLIP and provide > your > > feedback. > > > > Thanks! > > Dong and Zhipeng > > >
