[ML][Minor] Separate estimator and model params for read/write test.

[yanboliang]

What changes were proposed in this pull request?

Since we allow Estimator and Model not always share same params (see ALSParams and ALSModelParams), we should pass in test params for estimator and model separately in function testEstimatorAndModelReadWrite.

How was this patch tested?

Existing tests.

[SparkQA]

Test build #73846 has finished for PR 17151 at commit efff28b.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #73847 has finished for PR 17151 at commit 8f4f87e.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[sethah]

It would seriously reduce the amount of code changed (and therefore make this much easier to review :p ) to just add an extra constructor:

def testEstimatorAndModelReadWrite[
    E <: Estimator[M] with MLWritable, M <: Model[M] with MLWritable](
      estimator: E,
      dataset: Dataset[_],
      testParams: Map[String, Any],
      checkModelData: (M, M) => Unit): Unit = {
    testEstimatorAndModelReadWrite(estimator, dataset, testParams, testParams, checkModelData)
}

[yanboliang]

@sethah Thanks for your kindly remind. I was planing to write as your suggestion before I'm thinking:

• Actually all Model only need to extends a fraction of Params from Estimator, so all testEstimatorAndModelReadWrite(estimator, dataset, testParams, checkModelData) should be changed to testEstimatorAndModelReadWrite(estimator, dataset, testEstimatorParams, testModelParams, checkModelData) eventually. I explicitly write with the later way in test suites to remind developers should separate their estimator and model params when adding new algorithms' read/write test. I'm afraid that developers are not aware of the separation if they refer other test suites and find almost all test cases only pass in testParams.
• Though in the currently change, we pass in allParamSettings to both testEstimatorParams and testModelParams, this is because they share the same params set. Others like ALS will be pass in separate params. I think we should push forward to refactor *** and ***Model to separate their params, which could make models more succinct.
• If this is a public API, I totally agree with you. However, this is an internal auxiliary function, I think all test cases will need to pass in separate params eventually, so I settle a matter at one go.

This is my two cents, I'm still open to hear your thoughts. If you have strongly opinion, I can update according your suggestion. Thanks.

[sethah]

Good points. I still think it's better to just add the extra constructor, but I don't feel strongly about it. So we can proceed with whatever you feel is best. Thanks!

[sethah]

I prefer to let any refactoring of these tests happen as-needed. If there are specific cases that need to be done now, we should create JIRAs to track them.

[yanboliang]

Yeah, actually almost all models are extends lots of params which are not necessary, I'd like to remove these params for models as todo list. I'll create JIRAs to track them. I'll merge this first since it blocks #17117. Thanks.

[sethah]

LGTM

[sethah]

I prefer to let any refactoring of these tests happen as-needed. If there are specific cases that need to be done now, we should create JIRAs to track them.

[yanboliang]

Merged into master. Thanks for reviewing.