DataFrame: Add DateTime column type #6302

beccamc · 2022-08-24T17:30:27Z

Add the DateTime type as a PrimitiveDataFrameColumn. This will better allow conversion between IDataView and the DataFrame.

Some DateTime code exists already, like the DateTime computations. However, the partial implementation left bugs, particularly when trying to convert between IDataView and DataFrame.

I tried to follow the existing patterns but if this shouldn't be bucketed in with the PrimitiveDataFrameColumns I can make changes.

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.BinaryOperations.cs

codecov · 2022-08-24T18:52:27Z

Codecov Report

Merging #6302 (03b7b8a) into main (de9afb5) will increase coverage by 0.33%.
The diff coverage is 35.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6302      +/-   ##
==========================================
+ Coverage   68.39%   68.72%   +0.33%     
==========================================
  Files        1141     1171      +30     
  Lines      244820   248493    +3673     
  Branches    25405    26032     +627     
==========================================
+ Hits       167444   170777    +3333     
- Misses      70722    70988     +266     
- Partials     6654     6728      +74

Flag	Coverage Δ
Debug	`68.72% <35.76%> (+0.33%)`	⬆️
production	`63.16% <22.58%> (+0.31%)`	⬆️
test	`89.11% <100.00%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...imitiveDataFrameColumn.ReversedBinaryOperations.cs	`50.00% <ø> (ø)`
...a.Analysis/PrimitiveDataFrameColumnComputations.cs	`45.66% <0.00%> (-0.03%)`	⬇️
...ysis.Tests/DataFrameColumn.BinaryOperationTests.cs	`100.00% <ø> (ø)`
...FrameColumn.BinaryOperationAPIs.ExplodedColumns.cs	`7.59% <16.66%> (+0.02%)`	⬆️
...eColumn.BinaryOperationImplementations.Exploded.cs	`47.65% <16.66%> (-1.38%)`	⬇️
...lysis/PrimitiveDataFrameColumn.BinaryOperations.cs	`41.15% <18.00%> (+0.85%)`	⬆️
...ata.Analysis/PrimitiveDataFrameColumnArithmetic.cs	`48.98% <24.00%> (-0.51%)`	⬇️
...Microsoft.Data.Analysis/DateTimeDataFrameColumn.cs	`40.00% <40.00%> (ø)`
...icrosoft.Data.Analysis/PrimitiveDataFrameColumn.cs	`79.34% <87.50%> (+0.19%)`	⬆️
src/Microsoft.Data.Analysis/IDataView.Extension.cs	`95.91% <100.00%> (+0.17%)`	⬆️
... and 83 more

src/Microsoft.Data.Analysis/DateTimeComputation.cs

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.BinaryOperations.tt

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumnArithmetic.tt

src/Microsoft.Data.Analysis/PrimitiveColumnContainer.cs

beccamc · 2022-08-31T23:32:44Z

All the .tt files are re-generated!

LittleLittleCloud

LGTM

LittleLittleCloud · 2022-09-01T00:20:49Z

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumnComputations.tt

@@ -65,6 +65,8 @@ namespace Microsoft.Data.Analysis
    }

 <# foreach (TypeConfiguration type in typeConfiguration) { #>
+<# if (type.TypeName == "DateTime") {#>


Is this current plan that no computation function will be supported for DataTime

There is an existing file called DateTimeComputations that have all the date time computations. They aren't similar to the other primitive types so I just left it as is. I've added a comment here.

dakersnar

What is the purpose of the .tt files in this codebase? I'm not familiar with that extension.

beccamc · 2022-09-02T21:16:59Z

The .tt files are text generator files. Since there is so much repeated code for the data frames they generate a bunch of classes and methods. For example, the exploded API file.

beccamc · 2022-09-06T16:23:16Z

@tarekgh Could I get a review/approval on this PR please?

tarekgh · 2022-09-06T17:47:00Z

src/Microsoft.Data.Analysis/PrimitiveColumnContainer.cs

+                DataFrameBuffer<DateTime> newBuffer = new DataFrameBuffer<DateTime>();
+                ret.Buffers.Add(newBuffer);
+                newBuffer.EnsureCapacity(buffer.Length);
+                ReadOnlySpan<T> span = buffer.ReadOnlySpan;


T

does T will be always DateTime type ? if so, can you please assert for that?

can this help better here https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafe.as?view=net-6.0#system-runtime-compilerservices-unsafe-as-2(-0?

In general will be good if we can avoid the boxing here.

Apologies, this was leftover from an incorrect comparison of DateTime and Bool column. This isn't actually necessary for the DateTime column and I'm removing it.

tarekgh · 2022-09-06T17:58:12Z

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.BinaryOperations.cs

@@ -1445,6 +1493,7 @@ internal DataFrameColumn AndImplementation<U>(PrimitiveDataFrameColumn<U> column
                case Type uintType when uintType == typeof(uint):
                case Type ulongType when ulongType == typeof(ulong):
                case Type ushortType when ushortType == typeof(ushort):
+                case Type DateTimeType when DateTimeType == typeof(DateTime):


what is really this code for? I mean what happen if we delete this code?

This comment apply to other similar code in this file.

All of this is auto-generated from the accompanying .tt file. All of these single line changes are generated off the additional DateTime type added here. I went with a "don't rock the boat" approach for this and just continued with the existing auto generated patterns.

Nothing would happen if we deleted this code, but deleting it requires going through the .tt file and finding all the correct places these pointless cases are generated, removing it, and ensuring it has no ill effects. I didn't consider it high priority but could be swayed if you believe it is.

I guess changing the order of the types would change the order of these cases and potentially break the experience. I guess that would a decent reason to remove these in favor of the default case.

I am not getting this. what order change can affect the behavior? it is a switch statement and testing the type against every case. you cannot have 2 cases equal same type. this whole switch statement can be written as simple if statement.

By the way, I am not asking to change it as part of this PR. but will be good if you can track cleaning up tt template files with some issue.

tarekgh · 2022-09-06T18:22:26Z

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumnArithmetic.cs

+                var span = mutableBuffer.Span;
+                for (int i = 0; i < span.Length; i++)
+                {
+                    ret[i] = (span[i] == scalar);


span[i] == scalar

How much we care comparing 2 DateTime objects which containing same number of ticks but created with different time zone? the remark https://docs.microsoft.com/en-us/dotnet/api/system.datetime.op_equality?view=net-6.0#remarks explain more about that.

This comment apply for other equality methods in this file too.

Good question here too. I don't think this is a concern until we hear customer feedback that they need better equality support.

Do we understand the user scenarios? How will the data get supplied? Changing that in the future can be a breaking change. Will be good if we understand the expectation and code for that.

tarekgh · 2022-09-06T18:37:35Z

I left a few comments here. Does using DateTime is expected to be a common scenario in ML.NET? DateTime in general can cause some issues if users using it combined with a time zone. Do we have any idea how users are going to use it?

In the runtime, to avoid such problem we introduced DateTimeOffset. I was going to suggest to use that instead of DateTime but the only concern with that is DateTimeOffset size is bigger than DateTime which can be issue with big data sets.

beccamc · 2022-09-07T23:29:28Z

@tarekgh Great question! The main goal for adding DateTime as a type is for parity with the IDataView (MB issue/ ML Issue). Right now this gap makes it hard to use the DataFrame as a way to process large amounts of data and then pass to ML.NET.

Time zones are always a pain for dates, but as we're just following the existing support/expectations of the DateTime type I don't think it'll be a problem. Since we are just trying to light up Data Frame <-> IDataView parity at this point good time zone support isn't necessary. If customers end up needing a lot of date time manipulation we could invest in the DateTimeOffset at that point.

tarekgh

I left a little comments. I'll leave it to you to decide if you are comfortable enough with the change to merge. Thanks for getting this done.

beccamc · 2022-09-08T18:13:06Z

/azp run

azure-pipelines · 2022-09-08T18:13:11Z

Commenter does not have sufficient privileges for PR 6302 in repo dotnet/machinelearning

LittleLittleCloud · 2022-09-08T18:14:53Z

/azp run

azure-pipelines · 2022-09-08T18:15:08Z

Azure Pipelines successfully started running 2 pipeline(s).

LittleLittleCloud · 2022-09-09T18:20:36Z

/azp run

azure-pipelines · 2022-09-09T18:20:50Z

Azure Pipelines successfully started running 2 pipeline(s).

LittleLittleCloud · 2022-09-13T17:16:19Z

/azp run

azure-pipelines · 2022-09-13T17:16:32Z

Azure Pipelines successfully started running 2 pipeline(s).

ericstj · 2022-09-14T20:19:39Z

So this is failing in some image-based tests on Ubuntu https://dev.azure.com/dnceng-public/public/_build/results?buildId=15034&view=ms.vss-test-web.build-test-results-tab&runId=290276&resultId=101333&paneView=dotnet-dnceng.dnceng-anon-build-release-tasks.helix-anon-test-information-tab

 System.ArgumentException : Parameter is not valid.
   at System.Drawing.SafeNativeMethods.Gdip.CheckStatus(Int32 status)
   at System.Drawing.Bitmap.UnlockBits(BitmapData bitmapdata)
   at Microsoft.ML.Transforms.Image.VectorToImageConvertingTransformer.Mapper.<>c__DisplayClass5_0`1.<GetterFromType>b__0(Bitmap& dst) in /__w/1/s/src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs:line 431
   at Microsoft.ML.Tests.ImageTests.TestBackAndForthConversionWithoutAlphaNoInterleaveNoOffset() in /__w/1/s/test/Microsoft.ML.Tests/ImagesTests.cs:line 804

We know that System.Drawing doesn't work well on non-Windows and it's no longer supported. @tarekgh would it make sense to just disable these tests since they're causing noise and re-enable once we have the System.Drawing replacement?

tarekgh · 2022-09-14T21:51:00Z

We know that System.Drawing doesn't work well on non-Windows and it's no longer supported. @tarekgh would it make sense to just disable these tests since they're causing noise and re-enable once we have the System.Drawing replacement?

I am wondering if such tests are flakey as we didn't disable them till now. We can temporary disable them if it is consistently failing the CI.

Becca McHenry added 4 commits August 9, 2022 16:49

add DateTime type

3cad50c

datetime computation changes

75e28fb

update primitivedataframe.tt

c45d398

pr cleanup

4c6c201

ghost added the community-contribution label Aug 24, 2022

beccamc commented Aug 24, 2022

View reviewed changes

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.BinaryOperations.cs Outdated Show resolved Hide resolved

LittleLittleCloud reviewed Aug 24, 2022

View reviewed changes

src/Microsoft.Data.Analysis/DateTimeComputation.cs Outdated Show resolved Hide resolved

update binaryoperations

e00dd2f

LittleLittleCloud reviewed Aug 31, 2022

View reviewed changes

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.BinaryOperations.tt Show resolved Hide resolved

LittleLittleCloud reviewed Aug 31, 2022

View reviewed changes

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumnArithmetic.tt Outdated Show resolved Hide resolved

re-generate all .tt files

6536a7e

beccamc commented Aug 31, 2022

View reviewed changes

src/Microsoft.Data.Analysis/PrimitiveColumnContainer.cs Outdated Show resolved Hide resolved

LittleLittleCloud approved these changes Sep 1, 2022

View reviewed changes

LittleLittleCloud reviewed Sep 1, 2022

View reviewed changes

add DateTime computation comment

f30925e

dakersnar reviewed Sep 1, 2022

View reviewed changes

tarekgh reviewed Sep 6, 2022

View reviewed changes

remove the unecessary CloneAsDateTime

998397a

add more equality test support

03b7b8a

tarekgh approved these changes Sep 7, 2022

View reviewed changes

beccamc mentioned this pull request Sep 14, 2022

DateTime type doesn't work with Forecasting scenario dotnet/machinelearning-modelbuilder#2166

Closed

LittleLittleCloud merged commit 50e5068 into dotnet:main Sep 20, 2022

ghost locked as resolved and limited conversation to collaborators Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataFrame: Add DateTime column type #6302

DataFrame: Add DateTime column type #6302

beccamc commented Aug 24, 2022

codecov bot commented Aug 24, 2022 •

edited

Loading

beccamc commented Aug 31, 2022

LittleLittleCloud left a comment

LittleLittleCloud Sep 1, 2022

beccamc Sep 1, 2022

dakersnar left a comment

beccamc commented Sep 2, 2022

beccamc commented Sep 6, 2022

tarekgh Sep 6, 2022

tarekgh Sep 6, 2022

beccamc Sep 7, 2022

tarekgh Sep 6, 2022

tarekgh Sep 6, 2022

beccamc Sep 7, 2022

beccamc Sep 7, 2022

tarekgh Sep 7, 2022

tarekgh Sep 7, 2022

tarekgh Sep 6, 2022

tarekgh Sep 6, 2022

beccamc Sep 7, 2022

tarekgh Sep 7, 2022

tarekgh commented Sep 6, 2022

beccamc commented Sep 7, 2022

tarekgh left a comment

beccamc commented Sep 8, 2022

azure-pipelines bot commented Sep 8, 2022

LittleLittleCloud commented Sep 8, 2022

azure-pipelines bot commented Sep 8, 2022

LittleLittleCloud commented Sep 9, 2022

azure-pipelines bot commented Sep 9, 2022

LittleLittleCloud commented Sep 13, 2022

azure-pipelines bot commented Sep 13, 2022

ericstj commented Sep 14, 2022

tarekgh commented Sep 14, 2022 •

edited

Loading

DataFrame: Add DateTime column type #6302

DataFrame: Add DateTime column type #6302

Conversation

beccamc commented Aug 24, 2022

codecov bot commented Aug 24, 2022 • edited Loading

Codecov Report

beccamc commented Aug 31, 2022

LittleLittleCloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakersnar left a comment

Choose a reason for hiding this comment

beccamc commented Sep 2, 2022

beccamc commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Sep 6, 2022

beccamc commented Sep 7, 2022

tarekgh left a comment

Choose a reason for hiding this comment

beccamc commented Sep 8, 2022

azure-pipelines bot commented Sep 8, 2022

LittleLittleCloud commented Sep 8, 2022

azure-pipelines bot commented Sep 8, 2022

LittleLittleCloud commented Sep 9, 2022

azure-pipelines bot commented Sep 9, 2022

LittleLittleCloud commented Sep 13, 2022

azure-pipelines bot commented Sep 13, 2022

ericstj commented Sep 14, 2022

tarekgh commented Sep 14, 2022 • edited Loading

codecov bot commented Aug 24, 2022 •

edited

Loading

tarekgh commented Sep 14, 2022 •

edited

Loading