[SPARK-17116][Pyspark] Allow parameters to be {string,value} dict at runtime

[aditya1702]

What changes were proposed in this pull request?

This pull allows a dictionary containing string as keys to be passed in the fit() method. Presently it only allows an instance of the Estimator to be passed as key.

How was this patch tested?

This patch was manually tested on a local PC.

[aditya1702]

@srowen@jaceklaskowski Could you take a look and check if this improvement is proper? Since this is my first contribution it might have some issues. 😅

[AmplabJenkins]

Can one of the admins verify this patch?

[HyukjinKwon]

Shouldn't we just use isinstance instead?

[aditya1702]

@HyukjinKwon I tried using isinstance(type(params.keys()[0],str) but it returned False. And why cant we directly check if it is a str or not?

[HyukjinKwon]

Ah, I meant, isinstance(params.keys()[0], str). Direct type comparison does not take care of subclasses. I believe it is a common pattern to check the types. For example,

>>>classA(str):
... pass
...
>>>isinstance(A(), str)
True>>>type(A()) isstrFalse

This might be a major problem in a way, for example, when you give an argument as OrderedDict which extends dict.

Another reason is consistency across codebase. I believe isinstance is more widely used in the codes.

[aditya1702]

@HyukjinKwon Ah I get you. I will make the changes

[HyukjinKwon]

I think we need a test here maybe and update the argument description if this change is legitimate.

[HyukjinKwon]

cc @holdenk

[aditya1702]

@HyukjinKwon this is my first time with contributing in Spark and so I didnt know if we add tests to each pull. If it is proper I will add the tests :)

[aditya1702]

@HyukjinKwon@holdenk@srowen I have updated the code. Could you please take a look

[HyukjinKwon]

Could we just make it just like the one as below?

params=dict([(getattr(self, p), v) forp, vinparams.items()])

BTW, I am not supposed to decide what should be added into Spark. We can wait for committer's review :)

[aditya1702]

@HyukjinKwon Ok sure.

[holdenk]

Thanks for working on this! So at first glance I'm not super sure if we want to add this (is this being done to be more closely related to the skicitlearn apis or something?)

A good next step would probably be adding some tests for this as well :) Once we've got some tests we can ping some of the Python ML people and see what they think.

[aditya1702]

@holdenk Sure. I will add the tests. I saw that as per the issue opened the setParams was taking in string dict while fit() method was not. Hence I thought it would be an improvement to do that to the fit() method as well.

[holdenk]

@aditya1702 - thanks that sounds like a good reason for us to be consistent between the two. Let me know how adding the tests goes :)

[HyukjinKwon]

Hi @aditya1702, do you mind if I ask whether you are still working on this? Maybe it should be closed for now if you are currently not able to work on this further. It seems inactive for few months.