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.

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

Conversation

deepyaman
Copy link
Collaborator

@deepyaman deepyaman commented Sep 16, 2024

Related to #135 and #136

Before this PR, it wasn't possible to call set_params() on Step objects, and Recipe.set_params() only trivially set the steps param. With this PR, you can actually set the parameters listed by Recipe.get_params() (and Step.get_params()), which is more robust and akin to scikit-learn.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.70%. Comparing base (cfd6dc1) to head (effadce).

Files with missing lines Patch % Lines
ibis_ml/core.py 94.28% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   87.37%   88.70%   +1.32%     
==========================================
  Files          27       27              
  Lines        1964     2009      +45     
==========================================
+ Hits         1716     1782      +66     
+ Misses        248      227      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deepyaman deepyaman marked this pull request as ready for review September 16, 2024 04:15
@deepyaman deepyaman self-assigned this Sep 16, 2024
ibis_ml/core.py Outdated Show resolved Hide resolved
ibis_ml/core.py Outdated Show resolved Hide resolved
)


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!

@deepyaman deepyaman force-pushed the feat/steps/set-params branch from 7b00324 to 7416c23 Compare September 16, 2024 14:59
@deepyaman deepyaman force-pushed the feat/steps/set-params branch from d69ed52 to 9cc99cf Compare September 16, 2024 15:32
@deepyaman deepyaman merged commit 1f5934e into ibis-project:main Sep 16, 2024
4 checks passed
@deepyaman deepyaman deleted the feat/steps/set-params branch September 16, 2024 16:22
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