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

Implement period ticklabelmode on cartesian date axes #4993

Merged
merged 13 commits into from
Jul 24, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 9, 2020

Resolves #4911.
Demo

@plotly/plotly_js

'corresponding ticks and grid lines.',
'Only has an effect for axes of `type` *date*',
'When set to *period*, tick labels are drawn in the middle of the period',
'between ticks.'
Copy link
Contributor

Choose a reason for hiding this comment

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

this is actually not quite the desired behaviour: if the ticks are spaced every 2 months and the labels are Jan and Mar, the Jan label should appear on Jan 15, not on Feb 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example... https://codepen.io/nicolaskruchten/pen/ZEQRBWw?editors=0010

In "instant" mode we see 4 "month" labels spaced two months apart. In "period" mode we probably should see only 3 labels, but they should remain Jan/Mar/May, and they should be positioned on Jan 15, Mar 15, May 15

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add a clearer codepen in the original issue

@alexcjohnson
Copy link
Collaborator

I said I was going to comment on #4911 with some more details, and I never did that, my apologies. But now that there's a PR, let me comment here in the context of your new mock. Here's the original - that has the same data and ranges as your new mock - thanks for using the same data, this makes it very clear what's going on!
Screen Shot 2020-07-10 at 9 08 51 AM
And here it is modified with ticklabelmode: 'period':
Screen Shot 2020-07-10 at 9 10 23 AM

First, it looks like the labels moved the wrong direction - ie right now the "1950" label appears between the 1900 and 1950 tick marks, it should be between 1950 and 2000.

Second, as @nicolaskruchten noted we don't want the labels in the middle of the range, but in the middle of the period DENOTED BY THAT LABEL. So the label "2000" doesn't go halfway between the ticks for 2000 and 2050, it goes halfway between the beginning of 2000 and the beginning of 2001. The important thing here is what's the smallest increment of time included in the label? If it's years, the label shifts half a year into the future. If it's months, the label shifts half a month. If it's days, the label shifts half a day. It doesn't matter if dtick is 2 or 3 or 6 months, or even 10 years - if the label specifies months and nothing smaller, it shifts by half a month.

This needs to work whether the tick format is automatic, specified by tickformat, or pulled out of tickformatstops, which likely means we're going to have to parse the relevant format string and find the smallest time unit specifier it includes. There's an edge case here that @nicolaskruchten brought up in conversation with me: what if dtick is smaller than this smallest date part? Like someone specifies tickformat='%b' (months) and then zooms in to days? It'll look weird, as we'll have repeated "Jan" tick labels, but I think the thing to do here is fall back on half of dtick - ie the label shift is half the SMALLER of: the smallest unit displayed or dtick.

Third, and this is the part I've discussed now separately with both @archmoj and @nicolaskruchten but never written down, I think 'period' should cease to apply once you get to minutes precision, and we treat such times as always instants. I think this has practical benefits, in that we display minutes very often when one minute is a quite small fraction of the range, so shifting by half a minute would end up just looking like we misaligned the label; and conceptually once you start displaying times like "12:00" my feeling is people naturally read these as instants and not periods.

What about hour precision? It's not part of our default formatting, that goes straight from days to minutes, so you won't get it by zooming in on a graph with automatic labels. You'll only see this if you explicitly specify a tickformat or tickformatstop with hours as the smallest unit. My feeling is people do think of "the 12 o'clock hour" as a period so if they've gone to the trouble of specifying this AND ticklabelmode='period' we should honor it.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 20, 2020

Changes made in 0e124c2 are reflected in the codepen at PR description.

@alexcjohnson
Copy link
Collaborator

Looking much better! The day and month label offsets look perfect. The only issue I see there is that sometimes there should be an extra label at the beginning (when the tick is off-plot but the label position is on-plot), and sometimes there should NOT be a label at the end (when the tick is on-plot and the label position is off-plot). See the top row in the below image.

Year tick labels have this same issue, but sometimes it looks like some of the labels get offset by half a year while others get offset by half a month? See the bottom row in the below image. When you drag this axis around sometimes a single tick will shift back and forth between half-month and half-year offsets, when it's near the end of the range.

And when minutes or smaller are shown, the labels still offset. This shift should not happen at all in that case. That's the middle three rows.

Screen Shot 2020-07-20 at 4 27 20 PM

Finally, if you set an explicit tickformat, the smallest field included in the format should determine the offset, NOT dtick. See the image below where I set tickformat: "%Y %b %d" so the offset should be half a day, despite dtick being one month.

Screen Shot 2020-07-20 at 4 42 02 PM

@archmoj archmoj force-pushed the fix4911-ticklable-positioning branch from 778f483 to a5e6aae Compare July 21, 2020 16:52
@archmoj archmoj changed the title Implement period ticklablemode on cartesian date axes Implement period ticklabelmode on cartesian date axes Jul 21, 2020
tickVals = [{
minor: false,
value: axes.tickIncrement(tickVals[0].value, ax.dtick, !axrev, ax.caldendar)
}].concat(tickVals);
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a big deal, but tickVals.unshift({...}) is slightly more efficient here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.
I also fixed reversed range positioning and added few tests in 83f5cdd.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Works great now! Very nice. 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(x|y)axis.ticklabelmode = instant | period
3 participants