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

Make target optional for NoVarianceDataCheck #3339

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

freddyaboulton
Copy link
Contributor

Pull Request Description

Fixes #3337


After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #3339 (d360db0) into main (f7853d5) will decrease coverage by 0.7%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3339     +/-   ##
=======================================
- Coverage   99.6%   99.0%   -0.6%     
=======================================
  Files        329     329             
  Lines      32000   32003      +3     
=======================================
- Hits       31870   31652    -218     
- Misses       130     351    +221     
Impacted Files Coverage Δ
...s/data_checks_tests/test_no_variance_data_check.py 100.0% <ø> (ø)
evalml/data_checks/no_variance_data_check.py 100.0% <100.0%> (ø)
evalml/automl/pipeline_search_plots.py 17.9% <0.0%> (-82.1%) ⬇️
...l/tests/automl_tests/test_pipeline_search_plots.py 23.3% <0.0%> (-76.7%) ⬇️
...ests/automl_tests/test_automl_search_regression.py 74.3% <0.0%> (-20.4%) ⬇️
.../automl_tests/test_automl_search_classification.py 83.8% <0.0%> (-12.6%) ⬇️
evalml/tests/automl_tests/test_automl_utils.py 91.1% <0.0%> (-8.9%) ⬇️
...lml/tests/automl_tests/test_iterative_algorithm.py 92.3% <0.0%> (-7.7%) ⬇️
evalml/automl/utils.py 98.5% <0.0%> (-1.5%) ⬇️
evalml/automl/automl_search.py 99.6% <0.0%> (-0.1%) ⬇️
... and 1 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 f7853d5...d360db0. Read the comment docs.

if y is not None:
y = infer_feature_types(y)
else:
y = infer_feature_types(pd.Series([1, 2]))
Copy link
Contributor

@chukarsten chukarsten Mar 1, 2022

Choose a reason for hiding this comment

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

Is this hardcoding the series of integers intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than adding an if below and having to indent all the target-related logic (or returning early), I figured I would just use a dummy target that is not "no variance"

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't think checking below and then returning early for the target-related logic is too bad of an idea but I'll leave it up to you guys!

I personally would be more okay hardcoding if this were in the tests and not in the implementation :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I am ok with it if it at least has a comment indicating why it's like that! For the future generations 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the switch to returning early!

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Left my thoughts on the hardcoding for impl that @chukarsten raised but whatever you guys decide SGTM. :shipit: 🚢 !

if y is not None:
y = infer_feature_types(y)
else:
y = infer_feature_types(pd.Series([1, 2]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't think checking below and then returning early for the target-related logic is too bad of an idea but I'll leave it up to you guys!

I personally would be more okay hardcoding if this were in the tests and not in the implementation :)

@freddyaboulton freddyaboulton force-pushed the 3337-no-variance-data-check-y-optional branch from 1960716 to b081feb Compare March 2, 2022 15:40
@freddyaboulton freddyaboulton enabled auto-merge (squash) March 2, 2022 15:43
@freddyaboulton freddyaboulton merged commit ef43744 into main Mar 2, 2022
@chukarsten chukarsten mentioned this pull request Mar 3, 2022
@freddyaboulton freddyaboulton deleted the 3337-no-variance-data-check-y-optional branch May 13, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoVarianceDataCheck should allow the target to be optional
3 participants