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 DateTime to DateTime standard conversion. #4273

Merged
merged 2 commits into from
Oct 2, 2019

Conversation

pieths
Copy link
Contributor

@pieths pieths commented Oct 1, 2019

This fixes an error which is caused by a missing DateTime to DateTime conversion when outputting DateTime columns in NimbusML. NimbusML calls in to RowCursorUtils.GetGetterAsCore (indirectly) which tries to find a conversion from DateTime to DateTime and fails with the following error:

Error: *** System.InvalidOperationException: 'No standard conversion from 'DateTime' to 'DateTime'

@pieths pieths requested a review from a team as a code owner October 1, 2019 23:28
@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #4273 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4273      +/-   ##
==========================================
+ Coverage   74.48%   74.57%   +0.08%     
==========================================
  Files         877      878       +1     
  Lines      153663   154026     +363     
  Branches    16828    16852      +24     
==========================================
+ Hits       114457   114863     +406     
+ Misses      34474    34426      -48     
- Partials     4732     4737       +5
Flag Coverage Δ
#Debug 74.57% <100%> (+0.08%) ⬆️
#production 70.16% <100%> (+0.08%) ⬆️
#test 89.52% <100%> (+0.03%) ⬆️
Impacted Files Coverage Δ
...est/Microsoft.ML.Core.Tests/UnitTests/DataTypes.cs 99.35% <100%> (+0.05%) ⬆️
src/Microsoft.ML.Data/Data/Conversion.cs 74.87% <100%> (+0.04%) ⬆️
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs 92.13% <0%> (-4.9%) ⬇️
src/Microsoft.ML.Dnn/DnnUtils.cs 66.08% <0%> (-0.87%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.6% <0%> (-0.33%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.7% <0%> (-0.21%) ⬇️
...st/Microsoft.ML.Functional.Tests/Explainability.cs 100% <0%> (ø) ⬆️
src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs 100% <0%> (ø) ⬆️
...soft.ML.Tests/PermutationFeatureImportanceTests.cs 100% <0%> (ø) ⬆️
src/Microsoft.ML.Dnn/TensorTypeExtensions.cs 81.81% <0%> (ø)
... and 8 more

@codemzs codemzs self-requested a review October 2, 2019 01:21
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

🕐

@@ -1673,5 +1674,9 @@ public void Convert(in TX src, ref SB dst)
public void Convert(in BL src, ref R8 dst) => dst = System.Convert.ToDouble(src);
public void Convert(in BL src, ref BL dst) => dst = src;
#endregion FromBL

#region ToDT
public void Convert(in DT src, ref DT dst) => dst = src;
Copy link
Member

@codemzs codemzs Oct 2, 2019

Choose a reason for hiding this comment

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

public void Convert(in DT src, ref DT dst) => dst = src; [](start = 8, length = 56)

Does not have test coverage as per https://codecov.io/gh/dotnet/machinelearning/pull/4273/diff #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a unit test for the new DateTime to DateTime conversion method.


In reply to: 330341048 [](ancestors = 330341048)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, PH!


In reply to: 330650008 [](ancestors = 330650008,330341048)

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

@ganik ganik merged commit e19369b into dotnet:master Oct 2, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants