Skip to content

[SPARK-21685][PYTHON][ML] PySpark Params isSet state should not change after transform #18982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BryanCutler wants to merge 15 commits into from
Closed

[SPARK-21685][PYTHON][ML] PySpark Params isSet state should not change after transform #18982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bbe3ef7
added regression test for preserving Param set state when transferrin…
BryanCutler Aug 17, 2017
1203056
changed _transfer_params_to_java to not set param in Java unless expl…
BryanCutler Aug 17, 2017
6f97a9d
Merge remote-tracking branch 'upstream/master' into pyspark-ml-param-…
BryanCutler Sep 6, 2017
482c025
added transform to test
BryanCutler Sep 6, 2017
1a9fa46
added python friendly method to set a single Java ParamPair
BryanCutler Sep 9, 2017
088ee52
added MiMa exclude
BryanCutler Sep 9, 2017
fa4f974
Merge remote-tracking branch 'upstream/master' into pyspark-ml-param-…
BryanCutler Nov 20, 2017
bef3fb5
updated MimaExcludes
BryanCutler Nov 20, 2017
bbd2144
Merge remote-tracking branch 'upstream/master' into pyspark-ml-param-…
BryanCutler Feb 8, 2018
339c793
explicit calls to param checks
BryanCutler Feb 8, 2018
9162944
Merge remote-tracking branch 'upstream/master' into pyspark-ml-param-…
BryanCutler Mar 21, 2018
037ebf7
Revert "added MiMa exclude"
BryanCutler Mar 21, 2018
32ab60d
Revert "added python friendly method to set a single Java ParamPair"
BryanCutler Mar 21, 2018
24a1dbf
cleanup and add test for default transfer
BryanCutler Mar 21, 2018
2eaf1a7
cleanup
BryanCutler Mar 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion python/pyspark/ml/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ def test_property(self):
raise RuntimeError("Test property to raise error when invoked")


class ParamTests(PySparkTestCase):
class ParamTests(SparkSessionTestCase):

def test_copy_new_parent(self):
testParams = TestParams()
Expand Down Expand Up @@ -514,6 +514,24 @@ def test_logistic_regression_check_thresholds(self):
LogisticRegression, threshold=0.42, thresholds=[0.5, 0.5]
)

def test_preserve_set_state(self):
dataset = self.spark.createDataFrame([(0.5,)], ["data"])
binarizer = Binarizer(inputCol="data")
self.assertFalse(binarizer.isSet("threshold"))
binarizer.transform(dataset)
binarizer._transfer_params_from_java()
self.assertFalse(binarizer.isSet("threshold"),
"Params not explicitly set should remain unset after transform")

def test_default_params_transferred(self):
dataset = self.spark.createDataFrame([(0.5,)], ["data"])
binarizer = Binarizer(inputCol="data")
# intentionally change the pyspark default, but don't set it
binarizer._defaultParamMap[binarizer.outputCol] = "my_default"
result = binarizer.transform(dataset).select("my_default").collect()
self.assertFalse(binarizer.isSet(binarizer.outputCol))
self.assertEqual(result[0][0], 1.0)

@staticmethod
def check_params(test_self, py_stage, check_params_exist=True):
"""
Expand Down
13 changes: 10 additions & 3 deletions python/pyspark/ml/wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,18 @@ def _transfer_params_to_java(self):
"""
Transforms the embedded params to the companion Java object.
"""
paramMap = self.extractParamMap()
pair_defaults = []
for param in self.params:
if param in paramMap:
pair = self._make_java_param_pair(param, paramMap[param])
if self.isSet(param):
pair = self._make_java_param_pair(param, self._paramMap[param])
self._java_obj.set(pair)
if self.hasDefault(param):
pair = self._make_java_param_pair(param, self._defaultParamMap[param])
pair_defaults.append(pair)
if len(pair_defaults) > 0:
sc = SparkContext._active_spark_context
pair_defaults_seq = sc._jvm.PythonUtils.toSeq(pair_defaults)
self._java_obj.setDefault(pair_defaults_seq)
Copy link
Member

@viirya viirya Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If java side and python side the default params are the same, do we still need to set default params for the java object? Are't they already set in java object if they are default params?

Copy link
Member Author

@BryanCutler BryanCutler Mar 23, 2018

Choose a reason for hiding this comment

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

My take is that while they should be the same, it's still possible they might not be. The user could extend their own classes or it's quite easy to change in Python. Although we don't really support this, if there was a mismatch the user would probably just get bad results and it would be really hard to figure out why. From the Python API, it would look like it was one value but actually using another in Scala.

If you all think it's overly cautious to do this, I can take it out. I just thought it would be cheap insurance to just set these values regardless.

Copy link
Contributor

@holdenk holdenk Mar 23, 2018

Choose a reason for hiding this comment

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

I think this is reasonable, a few extra lines to avoid potential unwanted user surprise is worth it.


def _transfer_param_map_to_java(self, pyParamMap):
"""
Expand Down