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

Fix relationship path deserialization #665

Merged
merged 3 commits into from
Jul 12, 2019

Conversation

CJStadler
Copy link
Contributor

We were deserializing the AggregationFeatures.relationship_path as a list instead of a RelationshipPath.

Fixes #657

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #665 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   97.42%   97.42%   +<.01%     
==========================================
  Files         118      118              
  Lines        9539     9547       +8     
==========================================
+ Hits         9293     9301       +8     
  Misses        246      246
Impacted Files Coverage Δ
featuretools/feature_base/feature_base.py 97.63% <100%> (ø) ⬆️
...aturetools/tests/primitive_tests/test_agg_feats.py 99.38% <100%> (+0.01%) ⬆️

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 a3966fa...dee8d79. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #665 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   97.42%   97.42%   +<.01%     
==========================================
  Files         118      118              
  Lines        9539     9547       +8     
==========================================
+ Hits         9293     9301       +8     
  Misses        246      246
Impacted Files Coverage Δ
featuretools/feature_base/feature_base.py 97.63% <100%> (ø) ⬆️
...aturetools/tests/primitive_tests/test_agg_feats.py 99.38% <100%> (+0.01%) ⬆️

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 a3966fa...dee8d79. Read the comment docs.

@CJStadler CJStadler requested a review from kmax12 July 12, 2019 18:25
@@ -605,3 +608,11 @@ def pd_top3(x):
else:
for i1, i2 in zip(true_results.iloc[i], df.iloc[i]):
assert (pd.isnull(i1) and pd.isnull(i2)) or (i1 == i2)


def _assert_agg_feats_equal(f1, f2):
Copy link
Contributor

Choose a reason for hiding this comment

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

would just checking f1.unique_name() == f2.unique_name() be enough given the what the method is supposed to do? i don't see any harm in adding the other stuff but unique name should have all this info below included if it is working properly, i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally that should be enough, but in this case that would be True even if one of relationship_paths is a list and the other is a RelationshipPath. RelationshipPath mostly behaves like a list so most of the logic still works, and the line which triggered the error is only hit under specific conditions:
https://github.com/Featuretools/featuretools/blob/a3966fa3b3f078d237f0f27465d56c5f27778395/featuretools/feature_base/feature_base.py#L617-L621

I could just compare unique_name and relationship_path – I added the other comparisons to be careful but they don't seem necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. we can leave it as is for now

Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -605,3 +608,11 @@ def pd_top3(x):
else:
for i1, i2 in zip(true_results.iloc[i], df.iloc[i]):
assert (pd.isnull(i1) and pd.isnull(i2)) or (i1 == i2)


def _assert_agg_feats_equal(f1, f2):
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. we can leave it as is for now

@CJStadler CJStadler merged commit 6da8c8b into master Jul 12, 2019
@CJStadler CJStadler deleted the fix-relationship-path-deserialization branch July 12, 2019 20:07
@rwedge rwedge mentioned this pull request Aug 19, 2019
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.

Error running DFS after filtering feature definitions
2 participants