-
Notifications
You must be signed in to change notification settings - Fork 86
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
ReplaceNullableTypes Component #3090
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3090 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 315 317 +2
Lines 30547 30679 +132
=======================================
+ Hits 30447 30579 +132
Misses 100 100
Continue to review full report at Codecov.
|
75522be
to
980167b
Compare
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
583bde2
to
43aa896
Compare
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
02d8af0
to
cf845fc
Compare
@@ -85,6 +85,14 @@ installdeps: | |||
pip install --upgrade pip -q | |||
pip install -e . -q | |||
|
|||
.PHONY: installdeps-min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A convenience to help people debug the minimum dependency checks we have in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use this in the CI job? Maybe in a follow-up pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already done in the github workflows, specifically the min deps CI workflows. I just put it in here so people can build a test environment to run them. If there's a better way that people do it, I can take this out!
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Show resolved
Hide resolved
@@ -831,7 +832,8 @@ def test_component_has_random_seed(): | |||
assert "random_seed" in params | |||
|
|||
|
|||
def test_transformer_transform_output_type(X_y_binary): | |||
@pytest.mark.parametrize("component_class", _all_transformers()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just modified this test to move the for-loop to pytest parameters. Made it easier on test failure to understand which transformer component was failing.
type(y), | ||
y.name if isinstance(y, pd.Series) else None, | ||
) | ||
if component_class in [PolynomialDetrender, LogTransformer, LabelEncoder]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should all be de-indenting...
|
||
@pytest.mark.parametrize("nullable_types_properly_set", [True, False]) | ||
@pytest.mark.parametrize("input_type", ["ww", "pandas"]) | ||
def test_replace_nullable_types_integer_target( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, the boolean and integer target tests were together, but it resulted in too much branch-complexity and ultimately made the tests harder to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chukarsten Thank you for this! I think the behavior and testing is solid. I left some minor comments on how to stream-line the implementation.
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
@@ -85,6 +85,14 @@ installdeps: | |||
pip install --upgrade pip -q | |||
pip install -e . -q | |||
|
|||
.PHONY: installdeps-min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use this in the CI job? Maybe in a follow-up pr.
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/replace_nullable_types.py
Outdated
Show resolved
Hide resolved
36565bc
to
19f7178
Compare
…inal behavior of running through the infer_feature_types.
…lacenullable transformer and update tests.
… is set and properly.
93054a5
to
8ea2cce
Compare
Addresses #3057