Skip to content

Conversation

@yanboliang
Copy link
Contributor

@yanboliang yanboliang commented Apr 20, 2016

What changes were proposed in this pull request?

#11663 adds type conversion functionality for parameters in Pyspark. This PR find out the omissive Param that did not pass corresponding TypeConverter argument and fix them. After this PR, all params in pyspark/ml/ used TypeConverter.

How was this patch tested?

Existing tests.

cc @jkbradley @sethah

@SparkQA
Copy link

SparkQA commented Apr 20, 2016

Test build #56359 has finished for PR 12529 at commit 90f0f74.

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

@yanboliang yanboliang changed the title [Minor] [ML] [PySpark] All params should use TypeConverter [Minor] [ML] [PySpark] Fix omissive params which should use TypeConverter Apr 20, 2016
@holdenk
Copy link
Contributor

holdenk commented Apr 20, 2016

Minor nit: Maybe s/omissive/missing/ for clarity while reading the title?
If all of our Params use type converter, maybe we should remove the default param of none and expected type for 2.0 - the docs mention keeping expected type until 2.1 but that seems maybe a bit longer than necessary if we've completely switched all of our internal types over and while the Params API is technically public I think requiring an explicit type converter specified would be a reasonable change for the 2.0 time frame. Just a thought, what do @sethah & @jkbradley think?

@jkbradley
Copy link
Member

I'm fine with removing expectedType. I brought this up here [https://github.com//pull/12480], but that comment will probably be buried now. I'll create a JIRA: [https://issues.apache.org/jira/browse/SPARK-14768]

@jkbradley
Copy link
Member

@yanboliang Thanks for the PR, LGTM (though I agree with Holden's title nit).
I'll merge this with master now regardless

@asfgit asfgit closed this in 296c384 Apr 20, 2016
@yanboliang yanboliang deleted the typeConverter branch April 21, 2016 02:05
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.

4 participants