Hi ChenLiang, Thanks for taking the next step to flush this out.
As a whole: - naming and basic interface seems consistent with existing conventions. - names are descriptive. - references to the literature is provided. - functionality is complementary to the library. What is not clear to me is: - Does this function require PostGIS to also be installed? If yes, it would be better if we disable the function if PostGIS is not present rather than introduce PostGIS as a dependency. (Similar to what we do with our requirement on the xml module with our PMML export functionality). - What are the exact datatypes in the function definition for regression_location and prediction_location? - In the description it describes regression_location as "The length of regression_location must be equal to the length of source_table", which signals to me that it is likely intended to be a column of the source table? If not then how is this length represented? - You didn't mark regression_location as (optional). Due to the way Postgres functions work all optional arguments must come after all required arguments, so having a non-optional argument in the middle of the optional list must be avoided. - I haven't read through the literature, but it is not immediately clear to me why prediction_location is a parameter to gwregr_train() rather than gwregr_predict(). Can you provide a brief description to the way that prediction_location is used in the model and its relationship to training and prediction. Regards, Caleb
