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

Move list_model_families to avoid circular dependencies #959

Merged
merged 12 commits into from Jul 22, 2020

Conversation

dsherry
Copy link
Collaborator

@dsherry dsherry commented Jul 21, 2020

Notes here

Summary: we ran into a circular dependency issue in this code. We realized that since list_model_families is currently defined as an attribute of estimators, we should move that code into the components namespace, and that doing so would avoid issues with importing other utilities from the components namespace.

Building on work from #898 #911 #934

@dsherry dsherry added enhancement An improvement to an existing feature. refactor Work being done to refactor code. labels Jul 21, 2020
@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #959 into main will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #959      +/-   ##
==========================================
- Coverage   99.87%   99.87%   -0.01%     
==========================================
  Files         171      171              
  Lines        8783     8780       -3     
==========================================
- Hits         8772     8769       -3     
  Misses         11       11              
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.55% <ø> (ø)
evalml/model_family/utils.py 100.00% <ø> (ø)
evalml/model_family/__init__.py 100.00% <100.00%> (ø)
evalml/pipelines/components/utils.py 100.00% <100.00%> (ø)
evalml/tests/pipeline_tests/test_pipelines.py 100.00% <100.00%> (ø)

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 428351a...9317ce7. Read the comment docs.

@dsherry dsherry force-pushed the ds_911_circular_import branch from 457d095 to 50661ac Compare Jul 21, 2020
@dsherry dsherry marked this pull request as ready for review Jul 21, 2020
@@ -102,7 +102,7 @@ def __init__(self,
allowed_model_families to be ignored.

allowed_model_families (list(str, ModelFamily)): The model families to search. The default of None searches over all
model families. Run evalml.list_model_families("binary") to see options. Change `binary`
model families. Run evalml.pipelines.components.utils.allowed_model_families("binary") to see options. Change `binary`
Copy link
Collaborator Author

@dsherry dsherry Jul 21, 2020

Choose a reason for hiding this comment

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

I thought about adding this to the user guide but I like what's currently there.

@dsherry dsherry force-pushed the ds_911_circular_import branch from 50661ac to 9317ce7 Compare Jul 21, 2020
@dsherry
Copy link
Collaborator Author

dsherry commented Jul 21, 2020

Oof, codecov is complaining because I made a docstring change to a file with near-100% coverage. <0.01% project coverage change introduced by this PR. I plan to ignore that and merge.

Perhaps we should lower our codecov threshold...

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

@dsherry Looks good to me! I think its fine to merge without the project codecov test passing in this instance.

@dsherry dsherry merged commit f532c08 into main Jul 22, 2020
1 of 2 checks passed
@dsherry dsherry deleted the ds_911_circular_import branch Jul 22, 2020
@angela97lin angela97lin mentioned this pull request Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to an existing feature. refactor Work being done to refactor code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants