Skip to content

Bar length in milliseconds #4558

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

Closed
nicolaskruchten opened this issue Feb 6, 2020 · 18 comments · Fixed by #4900
Closed

Bar length in milliseconds #4558

nicolaskruchten opened this issue Feb 6, 2020 · 18 comments · Fixed by #4900
Assignees
Labels
bug something broken
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Feb 6, 2020

Right now we can specify bar lengths in milliseconds on date axes but this is interpreted in a browser-timezone-dependent manner, which seems like a clear bug that makes this feature unusable. See https://codepen.io/nicolaskruchten/pen/qBOKdQG ...

Basically, the following figure JSONs (which differ only in x) should probably have the same output, regardless of timezone of browser:

{
"data":[{"base":["2020-02-06 00:00"],"orientation":"h","type":"bar",
"x":[86400000],"y":[1]}],
"layout":{"xaxis":{"type":"date"}
}
}
{
"data":[{"base":["2020-02-06 00:00"],"orientation":"h","type":"bar",
"x":["1970-01-02 00:00"],"y":[1]}],
"layout":{"xaxis":{"type":"date"}}
}
@etpinard etpinard added the bug something broken label Feb 6, 2020
@nicolaskruchten nicolaskruchten added this to the v1.54.0 milestone Feb 17, 2020
@nicolaskruchten nicolaskruchten modified the milestones: v1.54.0, v1.55.0, v1.5x Mar 15, 2020
@nicolaskruchten
Copy link
Contributor Author

This is actually a more general problem than "bar lengths", as shown in this scatter example: https://codepen.io/nicolaskruchten/pen/xxwzGBe ... it seems that we interpret numbers in milliseconds as timezone-dependent in general.

In fact, it's even funnier without the base in bars... the length is correct, but the base is moved back! https://codepen.io/nicolaskruchten/pen/LYprVoP

@alexcjohnson
Copy link
Collaborator

Bars on date axes are meaningless with no base - there's no such thing as zero. So while that's indeed funny behavior, I'm not inclined toward any sort of action to define one behavior as "correct".

In the scatter case we've long held this to be an unfortunate early decision that we're now locked into until we either make a new major or add explicit time zone handling. The issue is folks constructing their data client side as new Date(...).getTime() which bakes in the time zone offset, then we just take it back out while plotting - ie new Date('1970-01-02 00:00') is expected to behave the same as '1970-01-02 00:00'. I suppose we could have a discussion about whether we still view that use case as significant enough that changing it would constitute a breaking change - I'd love to do this in a v2 but I'm hesitant to make that change in v1.

For bars with base though I'm 100% on board with considering our interpretation of milliseconds a bug. Using the JS Date machinery to construct bar lengths with some arbitrary base would be extremely weird, and "x milliseconds" has an obvious meaning that we're not following. We should handle this in such a way that the existing date-string behavior is preserved.

@nicolaskruchten
Copy link
Contributor Author

Bars on date axes are meaningless with no base - there's no such thing as zero

Well, we basically identify 1970-01-01 as zero in many contexts, so I could see us just reifying that...

@nicolaskruchten
Copy link
Contributor Author

For bars with base though I'm 100% on board with considering our interpretation of milliseconds a bug

What about when the base is also specified in milliseconds?

@alexcjohnson
Copy link
Collaborator

we basically identify 1970-01-01 as zero in many contexts, so I could see us just reifying that...

we could, but I don't really see the point - does that help anyone? We should just push people to choose an explicit base.

What about when the base is also specified in milliseconds?

Then it should have the legacy timezone-dependent behavior, matching all other absolute position values on date axes, unless and until we change that behavior for scatter etc. Otherwise if you try to align two traces to each other you'd need to know which uses the timezone offset and which doesn't.

I really think that until we get around to either implementing real time zone support or releasing a v2, we just need to keep the existing meaning of absolute numeric positions on date axes, and make it self-consistent. For new projects (once we fix the bar length bug part of this) the recommendation is straightforward: use strings for absolute dates and milliseconds for relative dates.

@nicolaskruchten
Copy link
Contributor Author

OK, let's tightly-scope this thing to just the cases when there is a base and the value is a number.

One thing that will come up then sometimes is that all your y values (in default orientation) would be numbers and all your base values would be dates... I think we should auto-detect the xaxis.type to date in this case... thoughts?

@nicolaskruchten nicolaskruchten modified the milestones: v1.55.0, v1.54.3 May 12, 2020
@archmoj
Copy link
Contributor

archmoj commented May 14, 2020

Let's move this to at least a minor.

@nicolaskruchten
Copy link
Contributor Author

@archmoj why?

@alexcjohnson
Copy link
Collaborator

One thing that will come up then sometimes is that all your y values (in default orientation) would be numbers and all your base values would be dates... I think we should auto-detect the xaxis.type to date in this case... thoughts?

Assuming you meant yaxis.type then yes, that sounds right. Basically, if there's a base on bar traces, use that for autotype rather than y (in default orientation 😉).

Let's move this to at least a minor.

As a tightly-scoped issue this is unambiguously a bug. Of course it will need careful testing but I think it's fine in a patch.

@nicolaskruchten
Copy link
Contributor Author

Assuming you meant yaxis.type

hehe yes! My original examples are in horizontal/timeline mode (because I see this as being useful for e.g. Gantt charts) but in default orientation, yaxis is what I mean!

Of course it will need careful testing but I think it's fine in a patch.

+1

@archmoj
Copy link
Contributor

archmoj commented May 19, 2020

@nicolaskruchten in your codepen the base are identical and the x values are different.
I thought we are going to fix that for only base and not x?
If that's the case, would you please update the pen?

@nicolaskruchten
Copy link
Contributor Author

Done.

@archmoj
Copy link
Contributor

archmoj commented May 20, 2020

Done.

@nicolaskruchten is it https://codepen.io/nicolaskruchten/pen/qBOKdQG?
I cannot figure out what is changed in there?

For bars with base though I'm 100% on board with considering our interpretation of milliseconds a bug. Using the JS Date machinery to construct bar lengths with some arbitrary base would be extremely weird, and "x milliseconds" has an obvious meaning that we're not following. We should handle this in such a way that the existing date-string behavior is preserved.

Hmm... This is still pretty confusing.
@alexcjohnson what is special about bars with base?

@nicolaskruchten
Copy link
Contributor Author

nicolaskruchten commented May 20, 2020

@archmoj bars with base on date axes are very important for making timelines, and right now their display is browser-timezone-dependent, meaning we cannot make Gantt charts.

@alexcjohnson
Copy link
Collaborator

If we want to be precise about the data we're encoding when we make a bar chart, the x values for a horizontal bar chart are intended to be the size of each bar, but we conflate this with the end position of each bar because of the implicit base=0. That's fine on numeric axes where zero is well-defined, so the size is always referenced to zero unless you provide a base to move the reference. But on a date axis:

  • There's no zero, so in principle bars are meaningless until we define both the base and the size. Internally javascript uses the unix convention of Jan 1 1970, but that's meaningless to users.
  • Once you do provide a base, the x attribute unambiguously means size, and using a date as the bar size is meaningless. Again, we can ascribe it meaning based on js internals but that has no relevance to anyone's data. However using a number as milliseconds is completely well-defined, so if the bar doesn't have the correct size that's a bug.

So the only 100% meaningful data users can give us for bars on date axes is base as date strings and size as numbers of milliseconds. If we ever implement #4559, an "end" parameter, then users could also specify both base and end as date strings. Anything else is just a workaround users may have used before we had the correct options; we should try to keep these working to the extent we can but not at the expense of these two well-defined cases.

@nicolaskruchten
Copy link
Contributor Author

I should add that today, if you specify x in millis with no base, you get a bar of the correct length, with the endpoint being timezone-dependent and the base being moved back before 1970-01-01 by an appropriate amount. So basically right now as @alexcjohnson says, specifying a base and x in millis is broken because by adding a base, I change the length of the bar by some timezone-dependent amount.

@archmoj
Copy link
Contributor

archmoj commented Jun 3, 2020

@archmoj
Copy link
Contributor

archmoj commented Jun 5, 2020

Closed via #4900.

@archmoj archmoj closed this as completed Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants