Skip to content

Conversation

@mengxr
Copy link
Contributor

@mengxr mengxr commented Apr 9, 2015

The design doc was posted on the JIRA page. Python changes will be in a follow-up PR. @jkbradley

  1. Use codegen for shared params.
  2. Move shared params to package ml.param.shared.
  3. Set default values in Params instead of in Param.
  4. Add a few methods to Params and ParamMap.
  5. Move schema handling to SchemaUtils from Params.
  • check visibility of the methods added

@SparkQA
Copy link

SparkQA commented Apr 9, 2015

Test build #29906 has finished for PR 5431 at commit 29b004c.

  • This patch fails to build.
  • This patch does not merge cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 9, 2015

Test build #29910 has finished for PR 5431 at commit 55be1f3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 9, 2015

Test build #29909 has finished for PR 5431 at commit d63b5cc.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@mengxr mengxr changed the title [WIP][SPARK-5957][ML] better handling of parameters [SPARK-5957][ML] better handling of parameters Apr 13, 2015
@SparkQA
Copy link

SparkQA commented Apr 13, 2015

Test build #30150 has finished for PR 5431 at commit eec2264.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@jkbradley
Copy link
Member

Should this method in Params be made abstract?

def validate(paramMap: ParamMap): Unit = {}

I just realized we haven't been using it, and making it abstract would force users to think about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for "params.clear"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Make sure that the input param is initialized before this gets called." will be unclear to new developers. Maybe move this to parameter-specific doc:

@param param:  Make sure that this param is initialized before this method gets called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jkbradley
Copy link
Member

@mengxr I added minor comment but don't have major ones. That's it for now!

@jkbradley
Copy link
Member

LGTM once tests pass

@SparkQA
Copy link

SparkQA commented Apr 14, 2015

Test build #30210 has finished for PR 5431 at commit 26ae2d7.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 14, 2015

Test build #30212 has finished for PR 5431 at commit d19236d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@asfgit asfgit closed this in 971b95b Apr 14, 2015
@mengxr
Copy link
Contributor Author

mengxr commented Apr 14, 2015

Merged into master.

asfgit pushed a commit that referenced this pull request Apr 16, 2015
Same as #5431 but for Python. jkbradley

Author: Xiangrui Meng <[email protected]>

Closes #5534 from mengxr/SPARK-6893 and squashes the following commits:

d3b519b [Xiangrui Meng] address comments
ebaccc6 [Xiangrui Meng] style update
fce244e [Xiangrui Meng] update explainParams with test
4d6b07a [Xiangrui Meng] add tests
5294500 [Xiangrui Meng] update default param handling in python
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants