Store LatLong columns as object dtype containing tuples by tamargrey · Pull Request #414 · alteryx/woodwork

tamargrey · 2020-11-24T20:57:24Z

Changes LatLong dtype to be object so that we can store latlongs as tuples for easy access to their values
Converts latlong columns to a standard format - a tuple of strings (or a list of strings for Koalas)
Closes Change LatLong from string to object dtype for faster access #316

tamargrey · 2020-11-24T21:05:32Z

+                self._series = dd.from_pandas(formatted_series, npartitions=self._series.npartitions)
+            elif ks and isinstance(self._series, ks.Series):
+                formatted_series = self._series.to_pandas().apply(_reformat_to_latlong, use_list=True)
+                self._series = ks.from_pandas(formatted_series)


Ideally, we could just use apply on the dd and ks dataframes, but with Dask, we don't know the format of the data in the column (the "meta" like how it gets used in featuretools here. ), and with Koalas, there was an issue with User Defined Functions (UDFs).

There might be a better way of doing this conversion to the standard format, so I'm open to any suggestions here.

Updated the dask apply to use apply directly on the Dask series (I wasn't thinking before, but of course we do know the desired format since the whole point of this is to get the standard tuple([str, str]) formatting.)

Still haven't found a better way to do the koalas apply, though.

tamargrey · 2020-11-24T21:10:13Z

+
+    error = re.escape("LatLongs must either be a tuple, a list, or a string representation of a tuple. (1,2) does not fit the criteria.")
+    with pytest.raises(ValueError, match=error):
+        _reformat_to_latlong("'(1,2)'")


I don't know how you'd get this case as an input, but the error message here is misleading, and I'm wondering if we might want to handle this case.

codecov · 2020-11-25T18:45:39Z

Codecov Report

Merging #414 (e4360b9) into main (e44cec6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #414    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           31        31            
  Lines         4065      4221   +156     
==========================================
+ Hits          4065      4221   +156

Impacted Files	Coverage Δ
woodwork/datacolumn.py	`100.00% <100.00%> (ø)`
woodwork/datatable.py	`100.00% <100.00%> (ø)`
woodwork/logical_types.py	`100.00% <100.00%> (ø)`
woodwork/serialize.py	`100.00% <100.00%> (ø)`
woodwork/tests/conftest.py	`100.00% <100.00%> (ø)`
woodwork/tests/datacolumn/conftest.py	`100.00% <100.00%> (ø)`
woodwork/tests/datacolumn/test_data_column.py	`100.00% <100.00%> (ø)`
woodwork/tests/datatable/test_datatable.py	`100.00% <100.00%> (ø)`
woodwork/tests/datatable/test_serialization.py	`100.00% <100.00%> (ø)`
woodwork/tests/test_utils.py	`100.00% <100.00%> (ø)`
... and 1 more

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 e44cec6...af8e3b5. Read the comment docs.

thehomebrewnerd

This is looking pretty good - one subject for discussion. In describe for LatLongs I think we are only including np.nan in our nan_count results, but we would not consider (np.nan, 43) or (np.nan, np.nan) as having missing values. I'm not sure what the right approach is here, but it sort of feels like (np.nan, np.nan) should be counted as missing. Not as sure about (np.nan, 43).

tamargrey · 2020-12-03T14:46:22Z

I'm not sure what the right approach is here, but it sort of feels like (np.nan, np.nan) should be counted as missing. Not as sure about (np.nan, 43).

My initial thought is that an interesting way to do that would be to have latlong columns be three columns in the describe df: one for latlong, one for lat, and one for long. And for the separated out lat and long values we could do a wider array of calculations since they're numeric. Then again, is it useful to know the average latitude or the standard dev?

For simplicity, though. I think conflating np.nan with (np.nan, np.nan) in our nan count works. But if, say, 80% of latitudes couldn't be calculated, it's misleading to say that 80% of the data is null, since, in theory, you could still get useful information about the longitude, which is why having the separate lat and long columns could be useful.

gsheni · 2020-12-03T21:28:40Z

@tamargrey I think it's very dataset dependent if average latitude is useful or not. So the best we can do from a Woodwork perspective is to provide the information. So I like your idea of having 3 columns for describing a LatLong. We can do this in a follow up MR. No need to worry about that now.

For the nan count, we can keep it simple for now, and see how its used. If I had to guess, I would say it going to depend on the user, and what they are looking for.

gsheni

lgtm

thehomebrewnerd

Looks good to me!

auto-assign Bot assigned tamargrey Nov 24, 2020

tamargrey commented Nov 24, 2020

View reviewed changes

Comment thread woodwork/tests/datatable/test_datatable.py

tamargrey commented Nov 24, 2020

View reviewed changes

Comment thread woodwork/utils.py

tamargrey commented Nov 24, 2020

View reviewed changes

Comment thread woodwork/tests/datatable/test_serialization.py

tamargrey requested a review from gsheni November 30, 2020 14:23

thehomebrewnerd suggested changes Nov 30, 2020

View reviewed changes

tamargrey force-pushed the tuple-latlong branch from 243cc7d to e288a60 Compare November 30, 2020 19:49

gsheni reviewed Nov 30, 2020

View reviewed changes

Comment thread woodwork/datatable.py

Comment thread woodwork/tests/conftest.py Outdated

Comment thread woodwork/tests/conftest.py Outdated

Comment thread woodwork/utils.py Outdated

tamargrey requested review from gsheni and thehomebrewnerd December 1, 2020 19:41

gsheni reviewed Dec 2, 2020

View reviewed changes

Comment thread woodwork/tests/datatable/test_serialization.py Outdated

Comment thread woodwork/tests/datatable/test_serialization.py Outdated

Comment thread woodwork/tests/test_utils.py Outdated

Comment thread woodwork/tests/datatable/test_serialization.py

tamargrey force-pushed the tuple-latlong branch 2 times, most recently from dd58591 to ae67d56 Compare December 2, 2020 20:33

thehomebrewnerd reviewed Dec 3, 2020

View reviewed changes

tamargrey force-pushed the tuple-latlong branch from ae67d56 to 537bca5 Compare December 4, 2020 14:56

tamargrey requested a review from gsheni December 4, 2020 16:16

gsheni approved these changes Dec 4, 2020

View reviewed changes

tamargrey force-pushed the tuple-latlong branch from 4bdec7f to 027add0 Compare December 7, 2020 17:16

tamargrey requested a review from thehomebrewnerd December 7, 2020 17:34

Tamar Grey added 6 commits December 7, 2020 14:08

Add outline

33a1793

Add basic latlong test

d89f5fc

Update serialization

0b0dfb5

first attempt at formatting

638084b

fix reformatter

1dde79f

More fixes - still has bad tuple reformatter

54683f8

Tamar Grey added 21 commits December 7, 2020 14:08

Use literal_eval for formatting

6dbfb2f

temp

2aee648

Fix column creation

09d0323

Clean up comments

1bf7b66

Stop changing underlying data on serialization

ea1bced

Get working for null values

6a6615c

Update documentation

b866acc

remove comments

1ba8450

Add release note

5094cf0

Remove print statements

278c0f2

Use apply directly on dask series

ebda55b

Always turn latlongs to strings for parquet

46cb744

Handle describe for latlongs

3da7110

Match mode format for koalas

e32aca3

Fix typos

5dafa07

Use floats instead of strings as latlong values

a474c19

cleanup

9954a64

consolidate test

c3f1deb

Fix after rebase

0df707f

handle all null latlongs as null value

21f7fda

Fix after rebase

79f9824

tamargrey force-pushed the tuple-latlong branch from 027add0 to 79f9824 Compare December 7, 2020 19:08

thehomebrewnerd approved these changes Dec 7, 2020

View reviewed changes

tamargrey merged commit 0748222 into main Dec 7, 2020

gsheni mentioned this pull request Dec 14, 2020

v0.0.7 #461

Merged

gsheni deleted the tuple-latlong branch December 22, 2020 21:37

Conversation

tamargrey commented Nov 24, 2020 • edited by gsheni Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamargrey Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

tamargrey Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamargrey Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Uh oh!

tamargrey commented Dec 3, 2020

Uh oh!

gsheni commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsheni left a comment

Choose a reason for hiding this comment

Uh oh!

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tamargrey commented Nov 24, 2020 •

edited by gsheni

Loading

codecov Bot commented Nov 25, 2020 •

edited

Loading

gsheni commented Dec 3, 2020 •

edited

Loading