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
Multi output primitive stackability #679
Conversation
…new tests, preliminary test of multi output now works though
Codecov Report
@@ Coverage Diff @@
## master #679 +/- ##
==========================================
- Coverage 97.51% 97.48% -0.04%
==========================================
Files 118 118
Lines 10035 10214 +179
==========================================
+ Hits 9786 9957 +171
- Misses 249 257 +8
Continue to review full report at Codecov.
|
…etools into multi-output
…into multi-output
…w user access to different outputs of multi output features and support stacking on that.
…into multi-output
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.
We should test serializing stacked multi-output features
Also test GroupByTransformFeatures stacking on multi-output features
featuretools/tests/primitive_tests/test_groupby_transform_primitives.py
Outdated
Show resolved
Hide resolved
featuretools/tests/primitive_tests/test_groupby_transform_primitives.py
Outdated
Show resolved
Hide resolved
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.
looking good. here are a few comments
@@ -135,6 +136,8 @@ def _add_feature_to_trie(self, trie, feature, approximate_feature_trie, | |||
sub_ignored_trie = approximate_feature_trie.get_node(feature.relationship_path) | |||
|
|||
for dep_feat in feature.get_dependencies(): | |||
if isinstance(dep_feat, MultiOutputFeature): |
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.
would it be possible/make sense to have this conversion to the base feature happen in the get dependencies call or would that break other usages of get_dependcies
?
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.
that would break other usages of get_dependencies
, particularly in the from_dictionary
usages, for that function should return a specific slice or multi output feature with the associated column number, not just the base feature of the multi output without the column
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.
This looks good to me. Nice work!
Adding support for multi output primitive stacking
Closes #593