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

Add LatLong transform primitives - GeoMidpoint, IsInGeoBox, CityblockDistance #1814

Merged
merged 14 commits into from Jan 3, 2022

Conversation

gsheni
Copy link
Contributor

@gsheni gsheni commented Dec 27, 2021

  • Add LatLong transform primitives
  • These are primitives already in our Predict NYC Taxi Trip open source demo

@gsheni gsheni self-assigned this Dec 27, 2021
@gsheni gsheni marked this pull request as ready for review December 27, 2021 18:49
@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #1814 (1909cfe) into main (ea50803) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 1909cfe differs from pull request most recent head 3ab1a5c. Consider uploading reports for the commit 3ab1a5c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1814      +/-   ##
==========================================
+ Coverage   98.67%   98.74%   +0.07%     
==========================================
  Files         141      142       +1     
  Lines       15722    15872     +150     
==========================================
+ Hits        15513    15673     +160     
+ Misses        209      199      -10     
Impacted Files Coverage Δ
...retools/primitives/standard/transform_primitive.py 100.00% <100.00%> (ø)
featuretools/primitives/utils.py 99.27% <100.00%> (+0.02%) ⬆️
...s/tests/primitive_tests/test_latlong_primitives.py 100.00% <100.00%> (ø)
featuretools/tests/conftest.py 100.00% <0.00%> (+0.40%) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.45% <0.00%> (+0.65%) ⬆️
...computational_backends/calculate_feature_matrix.py 99.15% <0.00%> (+0.84%) ⬆️

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 ea50803...3ab1a5c. Read the comment docs.

@gsheni
Copy link
Contributor Author

gsheni commented Dec 28, 2021

Ran some tests to determine the best nan approach to go with.
Screen Shot 2021-12-27 at 8 27 42 PM
I tested it at lower Series lengths too, and came away with the same results.

Jupyter notebook:
replace_latlong_nan_perf.ipynb.zip

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Do we also want to create a new issue to remove the extra checking that is in place in the primitive functions to handle a single nan value after Woodwork is updated to allow for a tuple of nans in a LatLong column?

@gsheni
Copy link
Contributor Author

gsheni commented Dec 28, 2021

@gsheni gsheni requested a review from tamargrey January 3, 2022 18:03
Copy link
Contributor

@tamargrey tamargrey left a comment

Choose a reason for hiding this comment

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

lgtm!

@gsheni gsheni enabled auto-merge (squash) January 3, 2022 18:18
@gsheni gsheni merged commit d13ae5e into main Jan 3, 2022
@gsheni gsheni deleted the add_latlong_primitives branch January 7, 2022 17:00
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.

None yet

3 participants