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

Updated training window error assertion for relative units #728

Merged
merged 10 commits into from Sep 6, 2019

Conversation

@christopherbunn
Copy link
Contributor

commented Sep 4, 2019

  • Updated the error assertion for query_by_values so that an error is asserted only when the training window is in observations #719
  • Added a test case for when a relative unit is used for the training window #618
  • Updated test_calcuate_feature_matrix.py for new error assertion message
@codecov

This comment has been minimized.

Copy link

commented Sep 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@3951bfe). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #728   +/-   ##
=========================================
  Coverage          ?   97.64%           
=========================================
  Files             ?      118           
  Lines             ?    10247           
  Branches          ?        0           
=========================================
  Hits              ?    10006           
  Misses            ?      241           
  Partials          ?        0
Impacted Files Coverage Δ
featuretools/synthesis/dfs.py 100% <ø> (ø)
featuretools/entityset/entity.py 96.09% <100%> (ø)
...utational_backend/test_calculate_feature_matrix.py 99.35% <100%> (ø)
featuretools/tests/synthesis/test_dfs_method.py 100% <100%> (ø)

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 3951bfe...b6bb5e6. Read the comment docs.

@christopherbunn christopherbunn requested a review from jeremyliweishih Sep 4, 2019
Copy link
Contributor

left a comment

LGTM. What do you think @rwedge?

Copy link
Contributor

left a comment

Would it be possible to add testing for edge cases like leap years? Otherwise LGTM!

Copy link
Contributor

left a comment

LGTM. @rwedge

assert feature_matrix_2.index.all([1, 2, 3, 4])
assert feature_matrix_3.index.all([2, 3, 4])
assert feature_matrix_4.index.all([2, 3, 4])
assert feature_matrix_4.index.all([2, 3])

This comment has been minimized.

Copy link
@rwedge

rwedge Sep 6, 2019

Collaborator

I don't think these assertions are working as intended. assert feature_matrix_4.index.all([2, 3, 4]) and assert feature_matrix_4.index.all([2, 3]) are both evaluating as True

christopherbunn and others added 2 commits Sep 6, 2019
@rwedge
rwedge approved these changes Sep 6, 2019
Copy link
Collaborator

left a comment

Looks good!

@christopherbunn christopherbunn merged commit 1de047b into master Sep 6, 2019
4 checks passed
4 checks passed
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
license/cla Contributor License Agreement is signed.
Details
test_all_python_versions Workflow: test_all_python_versions
Details
@kmax12 kmax12 deleted the dfs-training-window branch Sep 11, 2019
@rwedge rwedge referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.