Fix timezone fill issue by scaling 'width' with UTC dates, instead of local dates #48

Merged
merged 3 commits into from Mar 31, 2014

Conversation

Projects
None yet
4 participants
@trhodeos
Contributor

trhodeos commented Mar 6, 2014

What? Why?

Width calculation was weird around DST, primarily because we were using local dates and plugging them into a d3.utc time scale. I'm sure there is a better way to fix this issue, but I'm no pro w/ js dates :(

How was it tested?

Scrolled to March 8/9 boundary, and saw that there is not white space in the fill color.

Note: this fixes the issue w/ safari, mainly because of the findNearestHourForDate helper function I added.

@jebeck

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 7, 2014

Collaborator

I'm going to check this out tomorrow @trhodeos, thanks!

Collaborator

jebeck commented Mar 7, 2014

I'm going to check this out tomorrow @trhodeos, thanks!

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 8, 2014

Collaborator

Hmm, so there's still an issue in Safari.

Not the same, issue, but the rectangles don't line up with the designated hour ticks pre-switch to DST on March 9:
screenshot 2014-03-07 23 09 39

I'm scratching my head on this one too - I'll mull it over this weekend. Chrome and Safari generally behave the same (and Firefox is the odd one out) with respect to how they interpret timezone-naive dates, but clearly there is another difference between Chrome and Safari?!?

I suspect the ultimate solution to this may have to involve getting rid of any instances of getTimezoneOffset() (which makes my skin crawl anyway, relying on the browser's locale, although it's very much my fault it's in there in the first place!). And probably also a switch the the UTC versions of the JavaScript Date functions, e.g., getUTCHours() vs. getHours().

Or maybe this will help, which I admit I haven't played with yet: https://github.com/mbostock/d3/wiki/Time-Intervals

Thanks again!

Collaborator

jebeck commented Mar 8, 2014

Hmm, so there's still an issue in Safari.

Not the same, issue, but the rectangles don't line up with the designated hour ticks pre-switch to DST on March 9:
screenshot 2014-03-07 23 09 39

I'm scratching my head on this one too - I'll mull it over this weekend. Chrome and Safari generally behave the same (and Firefox is the odd one out) with respect to how they interpret timezone-naive dates, but clearly there is another difference between Chrome and Safari?!?

I suspect the ultimate solution to this may have to involve getting rid of any instances of getTimezoneOffset() (which makes my skin crawl anyway, relying on the browser's locale, although it's very much my fault it's in there in the first place!). And probably also a switch the the UTC versions of the JavaScript Date functions, e.g., getUTCHours() vs. getHours().

Or maybe this will help, which I admit I haven't played with yet: https://github.com/mbostock/d3/wiki/Time-Intervals

Thanks again!

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 8, 2014

Contributor

That is actually an issue in chrome on master as well.
issue

I'm pretty sure the issue is that we are not normalizing the hours in between these lines (here and here).

Note: by normalizing the hours, I mean making sure that getHours() % opts.duration == 0.

The d3.intervals looks like the golden ticket. I believe that will solve many of these issues. I'll take a look at that soon.

I'm pretty busy this weekend, but I can possibly get to it on Sunday.

Contributor

trhodeos commented Mar 8, 2014

That is actually an issue in chrome on master as well.
issue

I'm pretty sure the issue is that we are not normalizing the hours in between these lines (here and here).

Note: by normalizing the hours, I mean making sure that getHours() % opts.duration == 0.

The d3.intervals looks like the golden ticket. I believe that will solve many of these issues. I'll take a look at that soon.

I'm pretty busy this weekend, but I can possibly get to it on Sunday.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 8, 2014

Collaborator

The issue in Chrome on master is the issue we're trying to fix, or am I misunderstanding you? The gap between rectangles causes the misalignment. And on this PR, now I think we just have overlap (instead of a gap) causing the misalignment.

I just investigated more fully (and I apologize for missing this last night), but the issue actually is there on Chrome (and Firefox) with your code too, although not quite the same as Safari?!? (It "fixes itself" in Chrome and Firefox in a way it doesn't in Safari.)
screenshot 2014-03-08 14 34 41

I agree about the location of the problem in the code, although I think it's the getTimezoneOffset() in those lines that may be causing the problem (because timezone offset changes on either side of the switch to DST, but we (me, originally) are only querying for the browser's current offset). I think the only way to make this work is going to be somehow using UTC, which doesn't do DST at all.

If at any point you get totally sick of working on this particular issue, let me know, and we can find something else if you want. And don't worry about timing, this is not an urgent bugfix.

Collaborator

jebeck commented Mar 8, 2014

The issue in Chrome on master is the issue we're trying to fix, or am I misunderstanding you? The gap between rectangles causes the misalignment. And on this PR, now I think we just have overlap (instead of a gap) causing the misalignment.

I just investigated more fully (and I apologize for missing this last night), but the issue actually is there on Chrome (and Firefox) with your code too, although not quite the same as Safari?!? (It "fixes itself" in Chrome and Firefox in a way it doesn't in Safari.)
screenshot 2014-03-08 14 34 41

I agree about the location of the problem in the code, although I think it's the getTimezoneOffset() in those lines that may be causing the problem (because timezone offset changes on either side of the switch to DST, but we (me, originally) are only querying for the browser's current offset). I think the only way to make this work is going to be somehow using UTC, which doesn't do DST at all.

If at any point you get totally sick of working on this particular issue, let me know, and we can find something else if you want. And don't worry about timing, this is not an urgent bugfix.

@jebeck jebeck added this to the UCSF Pilot milestone Mar 11, 2014

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 20, 2014

Contributor

Hey @jebeck (also @cheddar), so I've been thinking about this a bit in my off time, and I think that I've come to an acceptable solution? Let me know what you think.

Problem: we want every consumer of tideline (and blip) to see their data the same, regardless of where they uploaded and accessed their data.

What's happening right now: everything is in pseudo-UTC, meaning that tideline is currently interpreting non-UTC times as UTC. This works semi-well, with the issues being the ones we see here. And we don't get to take advantage of tools that d3 provides for inconsistent timezones. Also, it becomes confusing because "UTC" isn't really UTC... if that makes sense.

My proposal: we should interpret everything as 'local' time (until there is a uploaded-data normalizer at least).That way we can use d3's 'd3.time.scale' (not '.utc'), and let d3 handle any weird timezone inconsistencies there are. There are some edge cases, like making sure that UTC times are interpreted correctly, but I think that can be alleviated in Watson.

I may be totally off base here, or you guys may have already had this discussion.. In either case, I'd love to hear your thoughts!

Contributor

trhodeos commented Mar 20, 2014

Hey @jebeck (also @cheddar), so I've been thinking about this a bit in my off time, and I think that I've come to an acceptable solution? Let me know what you think.

Problem: we want every consumer of tideline (and blip) to see their data the same, regardless of where they uploaded and accessed their data.

What's happening right now: everything is in pseudo-UTC, meaning that tideline is currently interpreting non-UTC times as UTC. This works semi-well, with the issues being the ones we see here. And we don't get to take advantage of tools that d3 provides for inconsistent timezones. Also, it becomes confusing because "UTC" isn't really UTC... if that makes sense.

My proposal: we should interpret everything as 'local' time (until there is a uploaded-data normalizer at least).That way we can use d3's 'd3.time.scale' (not '.utc'), and let d3 handle any weird timezone inconsistencies there are. There are some edge cases, like making sure that UTC times are interpreted correctly, but I think that can be alleviated in Watson.

I may be totally off base here, or you guys may have already had this discussion.. In either case, I'd love to hear your thoughts!

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 21, 2014

Collaborator

Thanks so much for putting so much thought into this @trhodeos. Even if, as you'll see, I disagree, this has been incredibly useful in terms of making me think hard about our approach and check to be sure that it is the one we want to stick with. Also, some of this connects with lengthy discussions we've had internally that you weren't present for, and I'm sorry for that - it was my bad not seeing how this "small" bug related so closely to some pretty big, low-level issues we've had to work out.

I've put up a bl.ocks that helps illustrate why I still believe the pseudo-UTC approach is what we want (and what I believe fill.js should be brought more fully in line with - it was written before the pseudo-UTC approach became our strategy). Would love to hear your thoughts/feedback/questions on the bl.ock, or see a fork of it with your suggestion(s).

Collaborator

jebeck commented Mar 21, 2014

Thanks so much for putting so much thought into this @trhodeos. Even if, as you'll see, I disagree, this has been incredibly useful in terms of making me think hard about our approach and check to be sure that it is the one we want to stick with. Also, some of this connects with lengthy discussions we've had internally that you weren't present for, and I'm sorry for that - it was my bad not seeing how this "small" bug related so closely to some pretty big, low-level issues we've had to work out.

I've put up a bl.ocks that helps illustrate why I still believe the pseudo-UTC approach is what we want (and what I believe fill.js should be brought more fully in line with - it was written before the pseudo-UTC approach became our strategy). Would love to hear your thoughts/feedback/questions on the bl.ock, or see a fork of it with your suggestion(s).

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 24, 2014

Contributor

Awesome write up! After reading that, it became very clear to me what to do. I couldn't wrap my head around the pseudo-UTC bit at first but after reading that write up, it couldn't have been clearer. I've pushed up the changes. They fix the width issue in all browsers, and the "undefined" block in safari.

The only issue with the pseudo-UTC solution, which you probably have a answer for, is there could be overlapping timestamps during the fall daylight savings time switch. But I'm guessing it is remedied by the fact that meters and pumps don't automatically update their times.

Contributor

trhodeos commented Mar 24, 2014

Awesome write up! After reading that, it became very clear to me what to do. I couldn't wrap my head around the pseudo-UTC bit at first but after reading that write up, it couldn't have been clearer. I've pushed up the changes. They fix the width issue in all browsers, and the "undefined" block in safari.

The only issue with the pseudo-UTC solution, which you probably have a answer for, is there could be overlapping timestamps during the fall daylight savings time switch. But I'm guessing it is remedied by the fact that meters and pumps don't automatically update their times.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 26, 2014

Collaborator

I'm glad the write-up helped @trhodeos! I'll take a look at the updated PR tomorrow.

Collaborator

jebeck commented Mar 26, 2014

I'm glad the write-up helped @trhodeos! I'll take a look at the updated PR tomorrow.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 27, 2014

Collaborator

Awesome, @trhodeos, this is almost there. You're missing the final fill segment in each view (this is very subtle with this particular demo data file for one-day view, but it's obvious in two-week view) because of how d3.time.hour.utc.range() is exclusive of endpoint. Can you just do a quick fix for that?

And despite GitHub telling us the merge can be automatic, there is unwanted behavior when I tested a merge because I had to go into fill.js recently and add code to give the fill an extra "gutter" to keep some of the smbg images from getting cut off at the edges. It was a bit tricky to figure out (and I forgot to warn you that I went into the code!), so I'm going to e-mail you the patch to apply after you update this branch by merging in master. Let me know if you have any trouble.

Collaborator

jebeck commented Mar 27, 2014

Awesome, @trhodeos, this is almost there. You're missing the final fill segment in each view (this is very subtle with this particular demo data file for one-day view, but it's obvious in two-week view) because of how d3.time.hour.utc.range() is exclusive of endpoint. Can you just do a quick fix for that?

And despite GitHub telling us the merge can be automatic, there is unwanted behavior when I tested a merge because I had to go into fill.js recently and add code to give the fill an extra "gutter" to keep some of the smbg images from getting cut off at the edges. It was a bit tricky to figure out (and I forgot to warn you that I went into the code!), so I'm going to e-mail you the patch to apply after you update this branch by merging in master. Let me know if you have any trouble.

@cheddar

This comment has been minimized.

Show comment
Hide comment
@cheddar

cheddar Mar 27, 2014

Contributor

Btw, Jana, another option on the re-merging thing is you can fork his repo into your own namespace, do the merge there and do another PR from that. It should end up closing this one too.

Contributor

cheddar commented Mar 27, 2014

Btw, Jana, another option on the re-merging thing is you can fork his repo into your own namespace, do the merge there and do another PR from that. It should end up closing this one too.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 27, 2014

Collaborator

Oh, and I nearly forgot about the CLA: before we can merge the pull request, @trhodeos, we need you to agree to our CLA at http://tidepool-org.github.io/TidepoolCLA.pdf. If you are working on your own behalf and agree to the CLA, responding here with a comment of "I agree to the terms of the Tidepool CLA at http://tidepool-org.github.io/TidepoolCLA.pdf" is sufficient. If you are working on behalf of a company, we will need your company to fill out the CLA as a corporate entity and submit it to us. (If you've already done this and I missed it, point me to it? But otherwise I don't think we had this in place yet for your last PR.)

Collaborator

jebeck commented Mar 27, 2014

Oh, and I nearly forgot about the CLA: before we can merge the pull request, @trhodeos, we need you to agree to our CLA at http://tidepool-org.github.io/TidepoolCLA.pdf. If you are working on your own behalf and agree to the CLA, responding here with a comment of "I agree to the terms of the Tidepool CLA at http://tidepool-org.github.io/TidepoolCLA.pdf" is sufficient. If you are working on behalf of a company, we will need your company to fill out the CLA as a corporate entity and submit it to us. (If you've already done this and I missed it, point me to it? But otherwise I don't think we had this in place yet for your last PR.)

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 30, 2014

Contributor

Ok, I just rebased my branch onto tidepool-org/tideline's master. Didn't need to apply the patch! And I added the fix for the exclusive endpoint as well. Forgot to even check the two-week view. I had to modify the 'gutter' code a bit. Because we are now adding fills from start to end (as opposed to end to start), some of the end fills overlapped where they shouldn't have.

Contributor

trhodeos commented Mar 30, 2014

Ok, I just rebased my branch onto tidepool-org/tideline's master. Didn't need to apply the patch! And I added the fix for the exclusive endpoint as well. Forgot to even check the two-week view. I had to modify the 'gutter' code a bit. Because we are now adding fills from start to end (as opposed to end to start), some of the end fills overlapped where they shouldn't have.

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 30, 2014

Contributor

Also, I agree to the terms of the Tidepool CLA at http://tidepool-org.github.io/TidepoolCLA.pdf

Contributor

trhodeos commented Mar 30, 2014

Also, I agree to the terms of the Tidepool CLA at http://tidepool-org.github.io/TidepoolCLA.pdf

@kentquirk

This comment has been minimized.

Show comment
Hide comment
@kentquirk

kentquirk Mar 30, 2014

Contributor

When you say "didn't need to apply the patch" do you mean that you saw no conflicts in git? Because as I understood it, Jana's patch was to fix a semantic conflict -- the code merged properly but didn't work.

Contributor

kentquirk commented Mar 30, 2014

When you say "didn't need to apply the patch" do you mean that you saw no conflicts in git? Because as I understood it, Jana's patch was to fix a semantic conflict -- the code merged properly but didn't work.

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 30, 2014

Contributor

I rebased my branch onto the tip of master (on the tidepool-org repo), and my fill.js matched the fill.js on master (in terms of the lines of code added by the patch). It makes sense though. git rebase reapplies my changes after pulling in the changes that @jebeck made, whereas git merge has no context as to when the changes were made.

Contributor

trhodeos commented Mar 30, 2014

I rebased my branch onto the tip of master (on the tidepool-org repo), and my fill.js matched the fill.js on master (in terms of the lines of code added by the patch). It makes sense though. git rebase reapplies my changes after pulling in the changes that @jebeck made, whereas git merge has no context as to when the changes were made.

+ function durationSegmentedDomain() {
+ var first = new Date(opts.endpoints[0]);
+ var last = new Date(opts.endpoints[1]);
+ // make sure we encapsulate the domain completely by padding the start and end with `opts.duration`

This comment has been minimized.

@jebeck

jebeck Mar 30, 2014

Collaborator

I think you're partially undoing the change I made to add the "gutter" here.

@jebeck

jebeck Mar 30, 2014

Collaborator

I think you're partially undoing the change I made to add the "gutter" here.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 30, 2014

Collaborator

Sorry about this again, but the rebase is not accomplishing the proper integration. Any chance you can go back, do the merge, apply the patch and go from there? Then the only thing to change is the final fill interval not getting added. Or just starting fresh on a new branch from current master with your original fill.js file fix (commit 8b543d2) + patch and additional fix for not adding final rectangle would be great.

The changes that I made are changes you're now (partially) undoing in additional code. For reference, the change I made to add a gutter (to keep the smbg images from getting cut off if they have a value of exactly midnight, etc.) makes it so that the fill is slightly larger than the domain: the ticks should no longer be at the edges:
screenshot 2014-03-30 13 57 47

Since dimensions are hard-coded into example.js now, this means the first and last fill rectangles in each row should be 117px and the remainder 109px.

I'm working for a while this afternoon, if you want to touch base at all in IRC.

Collaborator

jebeck commented Mar 30, 2014

Sorry about this again, but the rebase is not accomplishing the proper integration. Any chance you can go back, do the merge, apply the patch and go from there? Then the only thing to change is the final fill interval not getting added. Or just starting fresh on a new branch from current master with your original fill.js file fix (commit 8b543d2) + patch and additional fix for not adding final rectangle would be great.

The changes that I made are changes you're now (partially) undoing in additional code. For reference, the change I made to add a gutter (to keep the smbg images from getting cut off if they have a value of exactly midnight, etc.) makes it so that the fill is slightly larger than the domain: the ticks should no longer be at the edges:
screenshot 2014-03-30 13 57 47

Since dimensions are hard-coded into example.js now, this means the first and last fill rectangles in each row should be 117px and the remainder 109px.

I'm working for a while this afternoon, if you want to touch base at all in IRC.

@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 30, 2014

Contributor

My IRC setup has been screwy for a while, and I haven't had time to fix it..

Ah, I didn't realize that the dimensions are hardcoded in example.js. Because fill.js now draws the rectangles from left to right, instead of right to left, the right-most rectangle was overlapping the one next to it. So I removed the code about changing the 'x' and 'width' attributes for the last rectangle. I don't know what a better solution would be.. I can make the rectangle generation code go in reverse, that's not a huge deal. and it'll keep the rest of the code the same. I can probably push that up later tonight.

Contributor

trhodeos commented Mar 30, 2014

My IRC setup has been screwy for a while, and I haven't had time to fix it..

Ah, I didn't realize that the dimensions are hardcoded in example.js. Because fill.js now draws the rectangles from left to right, instead of right to left, the right-most rectangle was overlapping the one next to it. So I removed the code about changing the 'x' and 'width' attributes for the last rectangle. I don't know what a better solution would be.. I can make the rectangle generation code go in reverse, that's not a huge deal. and it'll keep the rest of the code the same. I can probably push that up later tonight.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 31, 2014

Collaborator

Yeah, the patch was just to correct for the reversing of the array. This is its diff:

--- a/js/plot/util/fill.js
+++ b/js/plot/util/fill.js
@@ -67,10 +67,6 @@ module.exports = function(pool, opts) {
       pushFillFor(range[i], range[i + 1]);
     }

-    if (opts.dataGutter) {
-      fills.shift();
-    }
-
     selection.selectAll('rect')
       .data(fills)
       .enter()
@@ -78,7 +74,7 @@ module.exports = function(pool, opts) {
       .attr({
         'x': function(d, i) {
           if (opts.dataGutter) {
-            if (i === fills.length  - 1) {
+            if (i === 0) {
               return d.x - opts.dataGutter;
             }
             else {
-- 
Collaborator

jebeck commented Mar 31, 2014

Yeah, the patch was just to correct for the reversing of the array. This is its diff:

--- a/js/plot/util/fill.js
+++ b/js/plot/util/fill.js
@@ -67,10 +67,6 @@ module.exports = function(pool, opts) {
       pushFillFor(range[i], range[i + 1]);
     }

-    if (opts.dataGutter) {
-      fills.shift();
-    }
-
     selection.selectAll('rect')
       .data(fills)
       .enter()
@@ -78,7 +74,7 @@ module.exports = function(pool, opts) {
       .attr({
         'x': function(d, i) {
           if (opts.dataGutter) {
-            if (i === fills.length  - 1) {
+            if (i === 0) {
               return d.x - opts.dataGutter;
             }
             else {
-- 
@trhodeos

This comment has been minimized.

Show comment
Hide comment
@trhodeos

trhodeos Mar 31, 2014

Contributor

Ah, sorry. Duh. Ok, I've applied the patch, and reverted my changes to the 'gutter' code.

Contributor

trhodeos commented Mar 31, 2014

Ah, sorry. Duh. Ok, I've applied the patch, and reverted my changes to the 'gutter' code.

@jebeck

This comment has been minimized.

Show comment
Hide comment
@jebeck

jebeck Mar 31, 2014

Collaborator

Thanks, this looks good! :D
Merging!

Collaborator

jebeck commented Mar 31, 2014

Thanks, this looks good! :D
Merging!

jebeck added a commit that referenced this pull request Mar 31, 2014

Merge pull request #48 from trhodeos/tr.fixTimezoneFill
Fix timezone fill issue by scaling 'width' with UTC dates, instead of local dates

@jebeck jebeck merged commit 71dab25 into tidepool-org:master Mar 31, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment