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

Deal with NaN values in LatLong columns #1007

Merged
merged 14 commits into from Jun 5, 2020
Merged

Deal with NaN values in LatLong columns #1007

merged 14 commits into from Jun 5, 2020

Conversation

thehomebrewnerd
Copy link
Collaborator

Closes #453

This PR serves as replacement for #693

If a single Nan value was present instead of a tuple in a LatLong columns, this could result in indexing errors when using the Haversine, Latitude or Longitude primitives. This PR fixes that issue.

A check was put in place during entity creation to replace any NaN values with (NaN, NaN). This check is also implemented in the Haversine, Latitude and Longitude primitives in case the primitive functions is called directly as in the doctests.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #1007 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
+ Coverage   98.28%   98.29%   +0.01%     
==========================================
  Files         121      121              
  Lines       12397    12474      +77     
==========================================
+ Hits        12184    12261      +77     
  Misses        213      213              
Impacted Files Coverage Δ
...retools/primitives/standard/transform_primitive.py 100.00% <100.00%> (ø)
featuretools/tests/entityset_tests/test_entity.py 100.00% <100.00%> (ø)
...s/tests/primitive_tests/test_transform_features.py 98.80% <100.00%> (+0.08%) ⬆️
...eaturetools/tests/utils_tests/test_entity_utils.py 100.00% <100.00%> (ø)
featuretools/utils/entity_utils.py 98.29% <100.00%> (+0.09%) ⬆️

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 0f35678...9f4c1b9. Read the comment docs.

jeff-hernandez
jeff-hernandez previously approved these changes Jun 2, 2020
Copy link
Contributor

@jeff-hernandez jeff-hernandez left a comment

Choose a reason for hiding this comment

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

Looks good!

featuretools/entityset/entity.py Outdated Show resolved Hide resolved
featuretools/utils/entity_utils.py Outdated Show resolved Hide resolved
rwedge
rwedge approved these changes Jun 5, 2020
Copy link
Collaborator

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

Looks good!

@thehomebrewnerd thehomebrewnerd merged commit a47673c into master Jun 5, 2020
3 checks passed
@thehomebrewnerd thehomebrewnerd deleted the issue453 branch June 5, 2020 18:10
@rwedge rwedge mentioned this pull request Jun 5, 2020
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.

haversine error
3 participants