-
Notifications
You must be signed in to change notification settings - Fork 89
Move default_parameters
to ComponentGraph from PipelineBase
#2307
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2307 +/- ##
=====================================
Coverage 99.9% 99.9%
=====================================
Files 281 281
Lines 24608 24607 -1
=====================================
- Hits 24579 24578 -1
Misses 29 29
Continue to review full report at Codecov.
|
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.
Nice!
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.
@angela97lin Looks good! I have a question about the release notes though.
@@ -33,6 +34,7 @@ Release Notes | |||
|
|||
**Breaking Changes** | |||
* Updated ``start_iteration_callback`` to accept a pipeline instance instead of a pipeline class and no longer accept pipeline parameters as a parameter :pr:`2290` | |||
* Moved ``default_parameters`` to ``ComponentGraph`` from ``PipelineBase``. A pipeline's ``default_parameters`` is now accessible via ``pipeline.component_graph.default_parameters`` :pr:`2307` |
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.
I think users would need to do pipeline._component_graph
. How come users need to access a private attribute to get the default parameters now? I think I missed the context for this change 🙈 Is there a plan to make pipeline.component_graph be a reference to the ComponentGraph
as opposed to list/dict representation?
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.
Yup, you're right!
Filed #2331, gonna work on it in a separate branch c:
(I believe its because we relied on the input component graph aka component_graph
to determine if linear but also wanted to store _component_graph
aka the underlying graph structure.)
Closes #2167