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

Component iteration in pipelines #1583

Merged
merged 16 commits into from
Jan 22, 2021
Merged

Component iteration in pipelines #1583

merged 16 commits into from
Jan 22, 2021

Conversation

eccabay
Copy link
Contributor

@eccabay eccabay commented Dec 18, 2020

Closes #1571

Obviously I'm not going to get this in before I leave today, but I had the time so I did what I could

Context since this is an old branch:

This PR addresses a breaking change made in #1543. Prior to #1543, the documented way to iterate through component instances was [c for c in pipeline.component_graph]. However, #1543 made it so that the component_graph attribute only lists the classes, not the instances. Rather than having users iterate through the private _component_graph attribute, we add a "public" accessor by making the pipelines iterable.

This PR accomplishes that by:

  1. Adding __iter__ and __next__ method to pipelines (as opposed to relying on __getitem__)
  2. Updating how we iterate through pipelines in code and docs. Let me know if I missed a spot hehe

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #1583 (af23165) into main (19b1b6e) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #1583     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         242      242             
  Lines       19075    19143     +68     
=========================================
+ Hits        19067    19135     +68     
  Misses          8        8             
Impacted Files Coverage Δ
evalml/pipelines/component_graph.py 100.0% <100.0%> (ø)
evalml/pipelines/pipeline_base.py 100.0% <100.0%> (ø)
evalml/pipelines/utils.py 100.0% <100.0%> (ø)
evalml/tests/pipeline_tests/test_pipelines.py 100.0% <100.0%> (ø)

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 19b1b6e...af23165. Read the comment docs.

@eccabay eccabay changed the base branch from 1278_pipelines_as_dags to main December 18, 2020 18:14
@eccabay eccabay marked this pull request as ready for review December 18, 2020 19:35
@dsherry dsherry marked this pull request as draft December 29, 2020 15:48
@freddyaboulton freddyaboulton force-pushed the 1571_pipeline_iteration branch from 9289ca2 to a742008 Compare January 21, 2021 23:02
@freddyaboulton freddyaboulton force-pushed the 1571_pipeline_iteration branch from a742008 to 2d4f2ce Compare January 21, 2021 23:03
@freddyaboulton freddyaboulton self-assigned this Jan 22, 2021
@freddyaboulton freddyaboulton added the enhancement An improvement to an existing feature. label Jan 22, 2021
@freddyaboulton freddyaboulton marked this pull request as ready for review January 22, 2021 16:30
Copy link
Collaborator

@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.

really cool! looks good to me.

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.

Looks great! Tests are solid, just curious what happens if index is not valid :P

def test_linear_getitem(logistic_regression_binary_pipeline_class):
pipeline = logistic_regression_binary_pipeline_class({'One Hot Encoder': {'top_n': 4}})

assert pipeline[0] == Imputer()
Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing! Just to confirm, giving an invalid index throws... an IndexError? Or ValueError? Just curious since get_component throws a ValueError :p

Copy link
Contributor

Choose a reason for hiding this comment

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

Great question, it throws an IndexError!

image

@freddyaboulton freddyaboulton added refactor Work being done to refactor code. and removed enhancement An improvement to an existing feature. labels Jan 22, 2021
@eccabay eccabay removed their assignment Jan 22, 2021
Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

This is super cool! LGTM!

@freddyaboulton freddyaboulton merged commit d96defd into main Jan 22, 2021
@bchen1116 bchen1116 mentioned this pull request Jan 26, 2021
@freddyaboulton freddyaboulton deleted the 1571_pipeline_iteration branch June 17, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Work being done to refactor code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support iterating over pipelines for component iteration
5 participants