Refactor ComponentGraph methods #2902
Conversation
| self._feature_provenance = self._get_feature_provenance(X.columns) | ||
| return self | ||
|
|
||
| def fit_features(self, X, y): | ||
| """Fit all components save the final one, usually an estimator. | ||
| def fit_and_transform_all_but_final(self, X, y): |
There was a problem hiding this comment.
I think it's fine to leave this method as is rather than trying to move it to time_series_base because it allows us to easily implement other pipeline types in the future that could benefit from this method. While we currently only use it for the time_series_base file, I believe the method makes sense existing where it currently is with the ComponentGraph class.
Additionally, I wouldn't want to delete this method and have time_series_base call the underlying private method since that would go against the design and structure of using private/public methods and naming conventions
bchen1116
requested review from
angela97lin,
chukarsten,
freddyaboulton,
christopherbunn,
dsherry,
eccabay,
jeremyliweishih and
ParthivNaresh
Codecov Report
@@ Coverage Diff @@
## main #2902 +/- ##
=====================================
Coverage 99.7% 99.7%
=====================================
Files 302 302
Lines 28394 28394
=====================================
Hits 28301 28301
Misses 93 93
Continue to review full report at Codecov.
|
docs/source/release_notes.rst
Outdated
| @@ -10,6 +10,7 @@ Release Notes | |||
| * Changes | |||
| * Deleted scikit-learn ensembler :pr:`2819` | |||
| * Refactored pipeline building logic out of ``AutoMLSearch`` and into ``IterativeAlgorithm`` :pr:`2854` | |||
| * Refactored names for methods in ``ComponentGraph`` :pr:`2902` | |||
| * Documentation Changes | |||
| * Updated ``install.ipynb`` to reflect flexibility for ``cmdstan`` version installation :pr:`2880` | |||
| * Testing Changes | |||
There was a problem hiding this comment.
could we add the API changes to breaking changes?
There was a problem hiding this comment.
@bchen1116 I think this looks good to me! I think the new names for the ComponentGraph methods make sense.
Maybe we should get @angela97lin 's thoughts on this before merge. One thing that comes to mind is that maybe we should change the name of the PipelineBase.compute_estimator_features to be transform_all_but_final? No need to do this change unless we all agree though.
There was a problem hiding this comment.
Thank you for doing this @bchen1116! Agreed with @freddyaboulton that I like transform_all_but_final but I see you've already made that change 😂
LGTM 🚢
fix #2795