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

commented Jul 12, 2019

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

Fixes #657

@codecov

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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 Jul 12, 2019

@@ -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):

This comment has been minimized.

Copy link
@kmax12

kmax12 Jul 12, 2019

Member

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.

This comment has been minimized.

Copy link
@CJStadler

CJStadler Jul 12, 2019

Author Contributor

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:

def relationship_path_name(self):
if self._path_is_unique:
return self.child_entity.id
else:
return self.relationship_path.name

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

This comment has been minimized.

Copy link
@kmax12

kmax12 Jul 12, 2019

Member

I see. we can leave it as is for now

@kmax12
kmax12 approved these changes Jul 12, 2019
Copy link
Member

left a comment

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):

This comment has been minimized.

Copy link
@kmax12

kmax12 Jul 12, 2019

Member

I see. we can leave it as is for now

@CJStadler CJStadler merged commit 6da8c8b into master Jul 12, 2019

4 checks passed

codecov/patch 100% of diff hit (target 97.42%)
Details
codecov/project 97.42% (+<.01%) compared to a3966fa
Details
license/cla Contributor License Agreement is signed.
Details
test_all_python_versions Workflow: test_all_python_versions
Details

@CJStadler CJStadler deleted the fix-relationship-path-deserialization branch Jul 12, 2019

@rwedge rwedge referenced this pull request Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.