Replaced calls to DateTime.Now with DateTime.UtcNow to be locale agnostic by Sorrien · Pull Request #133 · dotnet/machinelearning

Sorrien · 2018-05-11T19:05:03Z

The codebase now uses DateTime.UtcNow, instead of DateTime.Now, to be locale agnostic.
Fixes #110

The codebase now uses DateTime.UtcNow, instead of DateTime.Now, to be locale agnostic.

dnfclas · 2018-05-11T19:05:14Z

All CLA requirements met.

Ivanidzo4ka

TomFinley · 2018-05-11T19:16:40Z

Thanks so much @Sorrien ! Just requesting @KrzysztofCwalina take a peek to see if it does what he intends. :)

revoking review

Ivanidzo4ka · 2018-05-11T19:28:13Z

https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Core/Data/DateTime.cs#L204
I think that part need to be changed to dto.UtcDateTime as well.

Ivanidzo4ka · 2018-05-11T19:35:19Z

Thank you @Sorrien

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

Ivanidzo4ka

Sorrien · 2018-05-11T19:44:08Z

Should I be concerned about the Windows_NT Release? If so, how can I learn more about what went wrong? It says access denied when I click "details."

sharwell · 2018-05-11T19:45:46Z

❔ Is there any way to switch the code to using DateTimeOffset (which is comparable even in different time zones)?

Ivanidzo4ka · 2018-05-11T19:50:05Z

We apparently have issue which related with random seed and amount of threads in our tests, so it's not problem on your side. I will open issue regarding that, meanwhile, best i can suggest is retrigger build by adding space somewhere.

Regression
.Microsoft.ML.Scenarios.ScenariosTests.TrainAndPredictIrisModelWithStringLabelTest (from MSTestSuite)

Последняя 1 сборка провалились (С Провалилось#588 )
Заняло 0.46 секунд.
добавить описание
Стек вызовов
MESSAGE:
Assert.Equal() Failure
Expected: 1 (rounded from 1)
Actual: 0.99 (rounded from 0.994781792163849)

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

shauheen · 2018-05-11T19:53:56Z

You can simply trigger the tests by asking the @dotnet-bot to test Windows_NT Release please as a comment.

Sorrien · 2018-05-11T19:57:07Z

@sharwell I am not as familiar with DateTimeOffset but I will look into it.

DateTime.UtcNow is now DateTimeOffset.Now.UtcDateTime

Ivanidzo4ka · 2018-05-11T20:19:13Z

        private LassoFit GetLassoFit(IChannel ch, int maxAllowedFeaturesPerModel)
        {
-            DateTime startTime = DateTime.Now;
+            DateTime startTime = DateTimeOffset.Now.UtcDateTime;


DateTimeOffset.Now.UtcDateTime [](start = 33, length = 30)

Is there are any difference between DateTimeOffset.Now.UtcDateTime and DateTime.UtcNow? If no, why you prefer DateTimeOffset?

I've done further research into DateTimeOffset. As I understand it, there is no difference between DateTimeOffset.Now.UtcDateTime and DateTime.UtcNow, but you could access more information from DateTimeOffset like the timezone which would be more useful for storing dates for later. As we are using it here I can't find a difference, so I will undo these changes.

revoking review

Ivanidzo4ka · 2018-05-11T20:25:30Z

As you can see in PR we mostly use DateTime.something to output time stamps and get ticks. To represent datetime internally as part of IDataView we use DvDateTime (see DateTime.cs) and we use DateTimeOffset in it.
So I'm not sure DateTimeOffset necessary for this PR

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

Sorrien · 2018-05-11T20:26:33Z

I've done further research into DateTimeOffset. As I understand it, there is no difference between the result of DateTimeOffset.Now.UtcDateTime and DateTime.UtcNow, but you could access more information from DateTimeOffset like the timezone which would be more useful for storing dates for later. As we are using it here I can't find a difference, so I will undo these changes.

This reverts commit 0587855.

KrzysztofCwalina · 2018-05-11T20:33:33Z

I've done further research into DateTimeOffset. As I understand it, there is no difference between DateTimeOffset.Now.UtcDateTime and DateTime.UtcNow,

@tarekgh, is that your understanding as well? I am under the impression that UtcNow would be faster and more reliable as Now will get time zone info.

Ivanidzo4ka

sharwell · 2018-05-11T21:30:48Z

As I understand it, there is no difference between the result of DateTimeOffset.Now.UtcDateTime and DateTime.UtcNow

Ah, I was suggesting the use of DateTimeOffset.UtcNow instead of DateTime.UtcNow, and eliminating the use of DateTime altogether. DateTimeOffset is a type that does not suffer from the usage limitations leading to this pull request. If someone decides to use DateTimeOffset.Now and someone else uses DateTimeOffset.UtcNow, and a third person uses DateTimeOffset.Now from a different time zone than the first, everything will work together automatically.

TomFinley · 2018-05-11T22:07:56Z

            var success = TryValidateOffset(dto.Offset.Ticks, out _offset);
            Contracts.Assert(success);
-            _dateTime = ValidateDate(new DvDateTime(dto.DateTime), ref _offset);
+            _dateTime = ValidateDate(new DvDateTime(dto.UtcDateTime), ref _offset);


new DvDateTime(dto.UtcDateTime), ref _offset); [](start = 37, length = 46)

Interesting. I don't know that this is correct, singe this DateTimeZone exists to encode a local time including a datetime. So I think that this change here might be inappropriate and is in fact introducing data corruption.

Thank you for pointing that out @TomFinley I will revert this change.

TomFinley

🕐

TomFinley · 2018-05-11T22:09:20Z


-            TimeSpan duration = DateTime.Now - startTime;
+            TimeSpan duration = DateTime.UtcNow - startTime;
            ch.Info("Elapsed time for compression: {0}", duration);


This in particular is a bit weird and doesn't really have an effect I'd think. Probably harmless though, plus UtcNow is much faster anyway so may as well use it.

Honestly this code here ought to have been using stopwatch. #Closed

You're right, this has no effect. I have added a stopwatch here instead as per your suggestion

…t removed timezone info where it was actually needed

TomFinley · 2018-05-11T23:57:45Z

        {
-            DateTime startTime = DateTime.Now;
+            Stopwatch stopWatch = new Stopwatch();
+            stopWatch.Start();


Just a quick note, not major: Stopwatch.StartNew() is made to serve this common case where you both want to create and start a stopwatch. (Not a blocking comment to be clear, only if you happen to post another iteration for some other reason anyway.)

TomFinley

tarekgh · 2018-05-12T17:53:24Z

@KrzysztofCwalina

is that your understanding as well? I am under the impression that UtcNow would be faster and more reliable as Now will get time zone info.

Yes, DateTime,UtcNow is faster than DateTimeOffset.Now.

Sorrien · 2018-05-12T18:29:28Z

Thanks for working with me everyone. I hope to contribute more in the future.

shauheen · 2018-05-12T23:40:48Z

Thanks @Sorrien for your contribution and working through this PR. We look forward to that as well.

…stic (dotnet#133) * Removed calls to DateTime.Now The codebase now uses DateTime.UtcNow, instead of DateTime.Now, to be locale agnostic, except in cases where timezone info is actually needed. Also replaced one starttime measurement with stopwatch.

Removed calls to DateTime.Now

791444b

The codebase now uses DateTime.UtcNow, instead of DateTime.Now, to be locale agnostic.

Ivanidzo4ka previously approved these changes May 11, 2018

View reviewed changes

TomFinley requested a review from KrzysztofCwalina May 11, 2018 19:15

Replaced DateTime reference with UtcDatetime

030fa6f

Ivanidzo4ka previously approved these changes May 11, 2018

View reviewed changes

Replace DateTime with DateTimeOffset

0587855

DateTime.UtcNow is now DateTimeOffset.Now.UtcDateTime

Ivanidzo4ka reviewed May 11, 2018

View reviewed changes

Revert "Replace DateTime with DateTimeOffset"

da92df9

This reverts commit 0587855.

Ivanidzo4ka approved these changes May 11, 2018

View reviewed changes

TomFinley reviewed May 11, 2018

View reviewed changes

TomFinley suggested changes May 11, 2018

View reviewed changes

TomFinley reviewed May 11, 2018

View reviewed changes

Replaced starttime measurement with stopwatch and reverted change tha…

b6c5d50

…t removed timezone info where it was actually needed

TomFinley reviewed May 11, 2018

View reviewed changes

TomFinley approved these changes May 11, 2018

View reviewed changes

opted for the one liner version instantiating and starting the stopwatch

4266612

KrzysztofCwalina approved these changes May 12, 2018

View reviewed changes

TomFinley merged commit 5d03b54 into dotnet:master May 12, 2018

ghost locked as resolved and limited conversation to collaborators Mar 30, 2022

Uh oh!

Conversation

Sorrien commented May 11, 2018

Uh oh!

dnfclas commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley commented May 11, 2018

Uh oh!

Ivanidzo4ka commented May 11, 2018

Uh oh!

Ivanidzo4ka commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

Sorrien commented May 11, 2018

Uh oh!

sharwell commented May 11, 2018

Uh oh!

Ivanidzo4ka commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shauheen commented May 11, 2018

Uh oh!

Sorrien commented May 11, 2018

Uh oh!

Ivanidzo4ka May 11, 2018

Choose a reason for hiding this comment

Uh oh!

Sorrien May 11, 2018

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka commented May 11, 2018

Uh oh!

Sorrien commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrzysztofCwalina commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sorrien May 11, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sorrien May 11, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley May 11, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

tarekgh commented May 12, 2018

Uh oh!

Sorrien commented May 12, 2018

Uh oh!

shauheen commented May 12, 2018

Uh oh!

dnfclas commented May 11, 2018 •

edited

Loading

Ivanidzo4ka commented May 11, 2018 •

edited

Loading

Ivanidzo4ka commented May 11, 2018 •

edited

Loading

Sorrien commented May 11, 2018 •

edited

Loading

KrzysztofCwalina commented May 11, 2018 •

edited

Loading

sharwell commented May 11, 2018 •

edited

Loading

TomFinley May 11, 2018 •

edited

Loading

TomFinley May 11, 2018 •

edited

Loading