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

Clean up returned Codegen code #1371

Merged
merged 4 commits into from
Nov 5, 2020
Merged

Clean up returned Codegen code #1371

merged 4 commits into from
Nov 5, 2020

Conversation

bchen1116
Copy link
Contributor

@bchen1116 bchen1116 commented Oct 29, 2020

fix #1375

Added formatting for code generation (both for param dictionary and component graph). Remove unnecessary import code from generate_code_pipelines

@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #1371 into main will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
- Coverage   99.95%   99.95%   -0.00%     
==========================================
  Files         213      213              
  Lines       13938    13936       -2     
==========================================
- Hits        13931    13929       -2     
  Misses          7        7              
Impacted Files Coverage Δ
evalml/pipelines/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 9d43af7...02d1ba5. Read the comment docs.

@@ -170,9 +170,6 @@ def generate_pipeline_code(element):
raise ValueError("Element must be a pipeline instance, received {}".format(type(element)))

component_graph_string = ', '.join([com.__class__.__name__ if com.__class__ not in all_components() else "'{}'".format(com.name) for com in element.component_graph])
import_strings = [com.__class__.__name__ for com in element.component_graph if com.__class__ in all_components()]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this because for EvalML components, we don't need to import them in order to reference and use them in the component graph of a pipeline

@bchen1116 bchen1116 self-assigned this Oct 30, 2020
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

nice!

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

🚢 !

@@ -189,15 +188,15 @@ def generate_pipeline_code(element):
pipeline_string = "\t" + "\n\t".join(pipeline_list) + "\n" if len(pipeline_list) else ""
# create the base string for the pipeline
base_string = "\nclass {0}({1}):\n" \
"\tcomponent_graph = [{2}]\n" \
"\tcomponent_graph = [\n\t\t{2}\n\t]\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

'\nparameters = {\n\t"Imputer": {\n\t\t"categorical_impute_strategy": "most_frequent",\n\t\t"numeric_impute_strategy": "mean",\n\t\t"categorical_fill_value": None,\n\t\t"numeric_fill_value": None\n\t},' \
'\n\t"Random Forest Classifier": {\n\t\t"n_estimators": 100,\n\t\t"max_depth": 6,\n\t\t"n_jobs": -1\n\t}\n}\n' \
'pipeline = MockBinaryPipeline(parameters)'
pipeline = generate_pipeline_code(mock_binary_pipeline)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me

@dsherry
Copy link
Contributor

dsherry commented Nov 5, 2020

Arghh codecov!!! You only touched files with 100% coverage. I have no idea why codecov would complain about this... will look further.

Merging!

@dsherry dsherry merged commit 07ddabd into main Nov 5, 2020
@dsherry
Copy link
Contributor

dsherry commented Nov 5, 2020

Oops when I merged somehow my browser filled in the commit text as "Fix CLA" 🤦

@angela97lin
Copy link
Contributor

angela97lin commented Nov 6, 2020

@dsherry Codecov is failing because the total number of lines in evalml have decreased by 2, so the percentage overall decreased. I'm running into the same thing in #1388 😅

@dsherry dsherry mentioned this pull request Nov 24, 2020
@freddyaboulton freddyaboulton deleted the bc_fix_codegen branch May 13, 2022 14:58
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 formatting to Code Generation
6 participants