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

Add kwargs to all components #863

Merged
merged 6 commits into from Jun 18, 2020
Merged

Add kwargs to all components #863

merged 6 commits into from Jun 18, 2020

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Jun 18, 2020

fixes #775.

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #863 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #863   +/-   ##
=======================================
  Coverage   99.69%   99.70%           
=======================================
  Files         195      195           
  Lines        7962     8000   +38     
=======================================
+ Hits         7938     7976   +38     
  Misses         24       24           
Impacted Files Coverage Δ
evalml/tests/pipeline_tests/test_pipelines.py 99.75% <ø> (-0.01%) ⬇️
evalml/pipelines/components/component_base.py 100.00% <100.00%> (ø)
...ents/estimators/classifiers/baseline_classifier.py 100.00% <100.00%> (ø)
...ents/estimators/classifiers/catboost_classifier.py 100.00% <100.00%> (ø)
...ts/estimators/classifiers/elasticnet_classifier.py 100.00% <100.00%> (ø)
...components/estimators/classifiers/et_classifier.py 100.00% <100.00%> (ø)
...ents/estimators/classifiers/logistic_regression.py 100.00% <100.00%> (ø)
...components/estimators/classifiers/rf_classifier.py 100.00% <100.00%> (ø)
...nents/estimators/classifiers/xgboost_classifier.py 100.00% <100.00%> (ø)
...onents/estimators/regressors/baseline_regressor.py 100.00% <100.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce5de7d...89d89a2. Read the comment docs.

@jeremyliweishih jeremyliweishih marked this pull request as ready for review Jun 18, 2020
@jeremyliweishih jeremyliweishih requested review from dsherry and eccabay Jun 18, 2020
continue

obj_class = component._component_obj.__class__.__name__
module = component._component_obj.__module__
Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Jun 18, 2020

Choose a reason for hiding this comment

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

@dsherry __module__ was the key!

Copy link
Collaborator

@dsherry dsherry Jun 18, 2020

Choose a reason for hiding this comment

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

Amazing!!

@dsherry
Copy link
Collaborator

dsherry commented Jun 18, 2020

@jeremyliweishih is there a reason not to add **kwargs to ComponentBase.__init__? I think we should add that for consistency. I don't think we have transformer/estimator inits but same question there if so.

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Jun 18, 2020

@jeremyliweishih is there a reason not to add **kwargs to ComponentBase.__init__? I think we should add that for consistency. I don't think we have transformer/estimator inits but same question there if so.

@dsherry I can add it for consistency! It just wasn't actively doing anything in ComponentBase since the kwargs affects the component_objs. I wanted to add them to ComponentBase if there was a more general solution.

for k, v in kwargs.items():
setattr(self, k, v)

with patch(patched, new=all_init) as _:
Copy link
Collaborator

@dsherry dsherry Jun 18, 2020

Choose a reason for hiding this comment

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

@jeremyliweishih how come you specified new here? Why not let patch set up a MagicMock automatically?

with patch(patched) as mock_component_obj:
    component = component_class(test_arg="test")
    assert component.parameters['test_arg'] == "test"
    mock_component_obj.assert_called_with(test_arg="test")

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Jun 18, 2020

Choose a reason for hiding this comment

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

I don't have the exact error statement right now but MagicMock doesn't work directly with __init__ thus using the new=all_init. Before I had mocked_obj.__init__ = wtv and it didn't work.

Copy link
Collaborator

@dsherry dsherry left a comment

@jeremyliweishih got it, thanks. Yeah let's add **kwargs to ComponentBase.__init__/base class inits for consistency. Also I left a comment on the mocking in the unit test. Other than that LGTM!! Pretty cool that you figured out the dynamic mocking, great to have that pattern in place!

assert component._component_obj.test_arg == "test"


def test_component_has_random_state():
Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Jun 18, 2020

Choose a reason for hiding this comment

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

@dsherry this replaces the check in test_pipelines.py on if components have random_state as a parameter.

Copy link
Collaborator

@dsherry dsherry Jun 18, 2020

Choose a reason for hiding this comment

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

Awesome. Thanks

@jeremyliweishih jeremyliweishih merged commit d220dca into master Jun 18, 2020
2 checks passed
@angela97lin angela97lin mentioned this pull request Jun 30, 2020
@dsherry dsherry deleted the js_775_kwargs_ branch Oct 29, 2020
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.

Add kwargs to all component __init__ methods, and pass through to underlying models
2 participants