Skip to content
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

feat(core): implement a more robust set_params() #162

Merged
merged 3 commits into from
Sep 16, 2024
Merged

feat(core): implement a more robust set_params() #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7416c23
feat(core): implement a more robust `set_params()`
deepyaman Sep 15, 2024
9cc99cf
refactor(core): privatize step param getter/setter
deepyaman Sep 16, 2024
effadce
test(core): check that `Pipeline.set_params` works
deepyaman Sep 16, 2024
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
134 changes: 113 additions & 21 deletions ibis_ml/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,13 +347,8 @@ def _get_param_names(cls) -> list[str]:
# Extract and sort argument names excluding 'self'
return sorted([p.name for p in parameters])

def get_params(self, deep=True) -> dict[str, Any]:
"""Get parameters for this estimator.

Parameters
----------
deep : bool, default=True
Has no effect, because steps cannot contain nested substeps.
def _get_params(self) -> dict[str, Any]:
"""Get parameters for this step.

Returns
-------
Expand All @@ -370,6 +365,44 @@ def get_params(self, deep=True) -> dict[str, Any]:
"""
return {key: getattr(self, key) for key in self._get_param_names()}

def _set_params(self, **params):
"""Set the parameters of this step.

Parameters
----------
**params : dict
Step parameters.

Returns
-------
self : object
Step class instance.

Notes
-----
Derived from [1]_.

References
----------
.. [1] https://github.com/scikit-learn/scikit-learn/blob/74016ab/sklearn/base.py#L214-L256
"""
if not params:
# Simple optimization to gain speed (inspect is slow)
return self

valid_params = self._get_param_names()

for key, value in params.items():
if key not in valid_params:
raise ValueError(
f"Invalid parameter {key!r} for step {self}. "
f"Valid parameters are: {valid_params!r}."
)

setattr(self, key, value)

return self

def __repr__(self) -> str:
return pprint.pformat(self)

Expand Down Expand Up @@ -453,7 +486,7 @@ def _name_estimators(estimators):

class Recipe:
def __init__(self, *steps: Step):
self.steps = steps
self.steps = list(steps)
self._output_format = "default"

def __repr__(self):
Expand All @@ -465,16 +498,16 @@ def output_format(self) -> Literal["default", "pandas", "pyarrow", "polars"]:
return self._output_format

def get_params(self, deep=True) -> dict[str, Any]:
"""Get parameters for this estimator.
"""Get parameters for this recipe.

Returns the parameters given in the constructor as well as the
estimators contained within the `steps` of the `Recipe`.
steps contained within the `steps` of the `Recipe`.

Parameters
----------
deep : bool, default=True
If True, will return the parameters for this estimator and
contained subobjects that are estimators.
If True, will return the parameters for this recipe and
contained steps.

Returns
-------
Expand All @@ -493,18 +526,77 @@ def get_params(self, deep=True) -> dict[str, Any]:
if not deep:
return out

estimators = _name_estimators(self.steps)
out.update(estimators)
steps = _name_estimators(self.steps)
out.update(steps)

for name, estimator in estimators:
if hasattr(estimator, "get_params"):
for key, value in estimator.get_params(deep=True).items():
out[f"{name}__{key}"] = value
for name, step in steps:
for key, value in step._get_params().items(): # noqa: SLF001
out[f"{name}__{key}"] = value
return out

def set_params(self, **kwargs):
if "steps" in kwargs:
self.steps = kwargs.get("steps")
def set_params(self, **params):
"""Set the parameters of this recipe.

Valid parameter keys can be listed with ``get_params()``. Note that
you can directly set the parameters of the steps contained in
`steps`.

Parameters
----------
**params : dict
Parameters of this recipe or parameters of steps contained
in `steps`. Parameters of the steps may be set using its name and
the parameter name separated by a '__'.

Returns
-------
self : object
Recipe class instance.

Notes
-----
Derived from [1]_ and [2]_.

References
----------
.. [1] https://github.com/scikit-learn/scikit-learn/blob/ff1c6f3/sklearn/utils/metaestimators.py#L51-L70
.. [2] https://github.com/scikit-learn/scikit-learn/blob/74016ab/sklearn/base.py#L214-L256
"""
if not params:
# Simple optimization to gain speed (inspect is slow)
return self

# Ensure strict ordering of parameter setting:
# 1. All steps
if "steps" in params:
self.steps = params.pop("steps")

# 2. Replace steps with steps in params
estimator_name_indexes = {
x: i for i, x in enumerate(name for name, _ in _name_estimators(self.steps))
}
for name in list(params):
if "__" not in name and name in estimator_name_indexes:
self.steps[estimator_name_indexes[name]] = params.pop(name)

# 3. Step parameters and other initialisation arguments
valid_params = self.get_params(deep=True)

nested_params = defaultdict(dict) # grouped by prefix
for key, value in params.items():
key, sub_key = key.split("__", maxsplit=1)
if key not in valid_params:
raise ValueError(
f"Invalid parameter {key!r} for recipe {self}. "
f"Valid parameters are: ['steps']."
)

nested_params[key][sub_key] = value

for key, sub_params in nested_params.items():
valid_params[key]._set_params(**sub_params) # noqa: SLF001

return self

def set_output(
self,
Expand Down
6 changes: 3 additions & 3 deletions tests/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def test_mutate_at_expr():
res = step.transform_table(t)
sol = t.mutate(x=_.x.abs(), y=_.y.abs())
assert res.equals(sol)
assert list(step.get_params()) == ["expr", "inputs", "named_exprs"]
assert list(step._get_params()) == ["expr", "inputs", "named_exprs"] # noqa: SLF001


def test_mutate_at_named_exprs():
Expand All @@ -45,7 +45,7 @@ def test_mutate_at_named_exprs():
res = step.transform_table(t)
sol = t.mutate(x=_.x.abs(), y=_.y.abs(), x_log=_.x.log(), y_log=_.y.log())
assert res.equals(sol)
assert list(step.get_params()) == ["expr", "inputs", "named_exprs"]
assert list(step._get_params()) == ["expr", "inputs", "named_exprs"] # noqa: SLF001


def test_mutate():
Expand All @@ -56,4 +56,4 @@ def test_mutate():
res = step.transform_table(t)
sol = t.mutate(_.x.abs().name("x_abs"), y_log=lambda t: t.y.log())
assert res.equals(sol)
assert list(step.get_params()) == ["exprs", "named_exprs"]
assert list(step._get_params()) == ["exprs", "named_exprs"] # noqa: SLF001
54 changes: 53 additions & 1 deletion tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from unittest.mock import patch

import ibis
import ibis.expr.types as ir
import numpy as np
Expand Down Expand Up @@ -207,7 +209,8 @@ def test_can_use_in_sklearn_pipeline():

# get/set params works
params = p.get_params()
p.set_params(**params)
p.set_params(**params | {"recipe__scalestandard__inputs": ml.numeric()})
assert p["recipe"].steps[1].inputs == ml.numeric()

# fit and predict work
p.fit(X, y)
Expand Down Expand Up @@ -372,6 +375,55 @@ def test_get_params():
assert "expandtimestamp__components" not in rec.get_params(deep=False)


def test_set_params():
rec = ml.Recipe(ml.ExpandTimestamp(ml.timestamp()))

# Nonexistent parameter in step
with pytest.raises(
ValueError,
match="Invalid parameter 'nonexistent_param' for step ExpandTimestamp",
):
rec.set_params(expandtimestamp__nonexistent_param=True)

# Nonexistent parameter of pipeline
with pytest.raises(
ValueError, match="Invalid parameter 'expanddatetime' for recipe Recipe"
):
rec.set_params(expanddatetime__nonexistent_param=True)


def test_set_params_passes_all_parameters():
# Make sure all parameters are passed together to set_params
# of nested estimator.
rec = ml.Recipe(ml.ExpandTimestamp(ml.timestamp()))
with patch.object(ml.ExpandTimestamp, "_set_params") as mock_set_params:
rec.set_params(
expandtimestamp__inputs=["x", "y"],
expandtimestamp__components=["day", "year", "hour"],
)

mock_set_params.assert_called_once_with(
inputs=["x", "y"], components=["day", "year", "hour"]
)


def test_set_params_updates_valid_params():
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test that constructs a sklearn pipeline with a recipe, then calls the pipeline's get_params and set_params as sklearn would to ensure everything is plumbed through correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add anything here since test_can_use_in_sklearn_pipeline already covers that case (and was failing until got it "right").

Copy link
Member

Choose a reason for hiding this comment

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

That's probably the better place for it, but that test doesn't hit the setting nested parameters workflow directly. Can you add one that does something like:

pipe = ...
pipe.set_params(recipe__step__something="foo")
# then assert the nested thing was set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is hitting it, but I've made more explicit in effadce

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcrist if this looks good, I will go ahead and merge!

# Check that set_params tries to set `replacement_mutateat.inputs`, not
# `original_mutateat.inputs`.
original_mutateat = ml.MutateAt("dep_time", ibis._.hour() * 60 + ibis._.minute()) # noqa: SLF001
rec = ml.Recipe(
original_mutateat,
ml.MutateAt(ml.timestamp(), ibis._.epoch_seconds()), # noqa: SLF001
)
replacement_mutateat = ml.MutateAt("arr_time", ibis._.hour() * 60 + ibis._.minute()) # noqa: SLF001
rec.set_params(
**{"mutateat-1": replacement_mutateat, "mutateat-1__inputs": ml.cols("arrival")}
)
assert original_mutateat.inputs == ml.cols("dep_time")
assert replacement_mutateat.inputs == ml.cols("arrival")
assert rec.steps[0] is replacement_mutateat


@pytest.mark.parametrize(
("step", "url"),
[
Expand Down
Loading