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

Allow numeric datetime time index #282

Merged
merged 5 commits into from
Oct 19, 2020
Merged

Allow numeric datetime time index #282

merged 5 commits into from
Oct 19, 2020

Conversation

tamargrey
Copy link
Contributor

closes #275

We want to allow time indices to be made upon datatable init for columns such as [1,2,3] with the specified logical type Datetime.

@tamargrey tamargrey requested a review from gsheni October 19, 2020 15:55
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #282 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #282   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2508      2518   +10     
=========================================
+ Hits          2508      2518   +10     
Impacted Files Coverage Δ
woodwork/tests/data_table/test_datatable.py 100.00% <100.00%> (ø)
woodwork/utils.py 100.00% <100.00%> (ø)

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 7a07494...2d8e860. Read the comment docs.

assert dt.time_index == 'ints'
assert dt.to_pandas()['ints'].dtype == 'datetime64[ns]'

dt = DataTable(df, logical_types={'ints': Datetime})
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this was intentional, but at this point, dt['ints'] will be a bunch of datetime values, no longer integers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It look like this:

                            ints strs
0 1970-01-01 00:00:00.000000001    1
1 1970-01-01 00:00:00.000000002    2
2 1970-01-01 00:00:00.000000003    3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm fair point. The issue we're fixing here only shows up on init, but I was thinking it would be nice to have a test of the numeric datetimes column being set to the time index after creation, though it's no longer numerics. I see how it doesn't really add anything, though

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah if it doesn't add anything, lets remove the additional lines.

assert dt.to_pandas()['ints'].dtype == 'datetime64[ns]'

dt = DataTable(df, logical_types={'ints': Datetime})
dt = dt.set_time_index('ints')
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we trying to check here? If you can set a time index after creation?

@tamargrey tamargrey merged commit dd1ca2a into main Oct 19, 2020
@gsheni gsheni mentioned this pull request Oct 21, 2020
@gsheni gsheni deleted the fix-datetime-init branch November 13, 2020 20:11
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.

Cannot set time index to numeric column with logical type Datetime
2 participants