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

feat: make timeunit for bar/rect behaves more like bin #5096

Merged
merged 6 commits into from Jul 30, 2019
Merged

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented Jun 18, 2019

Fix #4868

In this PR, we only make timeUnit like binning for bar and rect.
We do not center other marks (e.g., point/line/area) in the middle of the timeUnit band by default for a few reasons:

  1. Area will be ugly, leaving empty spaces on the side

  2. Centering all marks so will cause breaking changes to all plots with timeUnit.
    By only making timeUnit behave like bin for just bars and rects, we only change the behaviors for plots that currently look terrible.

  3. Doing so will cause the plots with and without timeUnit look quite different.
    For example, if there is a date field with only year information. Using no timeUnit and timeUnit=year will produce dramatically different results.

When needed, users can easily set the band or mark config's timeUnitBand to 0.5 to center the data point in the middle of a time unit band. See line_month_center_band for example.

FWIW, Tableau also does NOT center the data point in the middle of a time unit band by default.

  • default alignment -- from an experiment the current one is already good

  • distinguish between band's width and position

    • pointBandSize, bandSize, bandPosition?
  • stack with impute case

  • document band

  • revise timeUnit docs

  • make band work for bar with temporal timeUnit will do in Band should also work with bar with bin / timeUnit #5243

Note that we don't include support for a field that has already been "prebinned" with a time unit transform. Let's handle that later in #5242.

@domoritz domoritz changed the title feat!: make timeunit behaves more like bin feat!: make timeunit behave more like bin Jun 19, 2019
@kanitw kanitw added the P1 Critical -- to fix ASAP label Jul 6, 2019
@kanitw kanitw force-pushed the kw/timeunit branch 2 times, most recently from bfda0fd to d275131 Compare July 19, 2019 23:44
kanitw added a commit that referenced this pull request Jul 20, 2019
…o avoid overflow

This is especially important for temporal in a follow up PR (#5096).
kanitw added a commit that referenced this pull request Jul 20, 2019
…o avoid overflow (#5204)

This is especially important for temporal in a follow up PR (#5096).
@kanitw kanitw force-pushed the kw/timeunit branch 2 times, most recently from 4ce82fe to 84c91a2 Compare July 26, 2019 00:30
@@ -13,6 +13,16 @@
"format": {"type": "csv", "parse": {"date": "date"}},
"transform": [
{"type": "identifier", "as": "_vgsid_"},
{
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if we generate the best time format here.

examples/compiled/interactive_seattle_weather.vg.json Outdated Show resolved Hide resolved
examples/compiled/layer_falkensee.vg.json Outdated Show resolved Hide resolved
examples/compiled/layer_line_errorband_ci.vg.json Outdated Show resolved Hide resolved
examples/compiled/area.vg.json Outdated Show resolved Hide resolved
examples/compiled/area_vertical.vg.json Outdated Show resolved Hide resolved
examples/compiled/bar_month.vg.json Outdated Show resolved Hide resolved
examples/compiled/bar_month_temporal.vg.json Show resolved Hide resolved
examples/compiled/bar_yearmonth.vg.json Show resolved Hide resolved
@kanitw kanitw force-pushed the kw/timeunit branch 9 times, most recently from cfc0497 to 68db08b Compare July 27, 2019 16:25
examples/compiled/line_color_binned.vg.json Outdated Show resolved Hide resolved
@@ -361,16 +363,50 @@ export interface PositionFieldDef<F extends Field>
impute?: ImputeParams;

/**
* For rect-based marks (`rect`, `bar`, and `image`), mark size relative to bandwidth of [band scales](https://vega.github.io/vega-lite/docs/scale.html#band). If set to `1`, the mark size is set to the bandwidth. If set to `0.5`, the mark size is half of the bandwidth.
* For rect-based marks (`rect`, `bar`, and `image`), mark size relative to bandwidth of [band scales](https://vega.github.io/vega-lite/docs/scale.html#band) or time units. If set to `1`, the mark size is set to the bandwidth or the time unit interval. If set to `0.5`, the mark size is half of the bandwidth or the time unit interval.
Copy link
Member Author

Choose a reason for hiding this comment

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

rect/bar can be band position too, if size property is explicitly specified.

@willium

This comment has been minimized.

@kanitw

This comment has been minimized.

@@ -55,15 +55,16 @@ export function rectPosition(model: UnitModel, channel: 'x' | 'y', mark: 'bar' |
// x, x2, and width -- we must specify two of these in all conditions
if (
isFieldDef(fieldDef) &&
(isBinning(fieldDef.bin) || isBinned(fieldDef.bin)) &&
// FIXME use band to determine condition here
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, this will be fixed later in #5244

@kanitw kanitw marked this pull request as ready for review July 28, 2019 03:29
@willium

This comment has been minimized.

@kanitw kanitw requested review from domoritz, Saba9 and znation and removed request for domoritz July 28, 2019 03:40
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Should we remove the grid so that it looks like in bins?

image

src/mark.ts Outdated
/**
* Default band position for a time unit.
*/
timeUnitBandPosition?: 0 | 0.5;
Copy link
Member

Choose a reason for hiding this comment

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

Why can this only be two values? Please document.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it doesn't have to be either 0 or 0.5. But in practice, these numbers won't make sense if they are not 0 (beginning of the timeUnit interval or 0.5 (middle of the timeUnit interval).

I can relax it to number.

@@ -17,9 +17,14 @@
"as": "yearmonth_date",
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the chart, it's really hard to know what the values for axis ticks are. Please revise this example.

Screen Shot 2019-07-29 at 9 30 21 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't change anything about the label.

it's really hard to know what the values for axis ticks are

This example is only there for explaining that you can customize the format. (See format.md.) So if you strongly think this should be change, please submit a separate PR for it.

@kanitw
Copy link
Member Author

kanitw commented Jul 30, 2019

Should we remove the grid so that it looks like in bins?

No. Grid in time actually usually represent year / month boundaries, which are helpful to readers. (Grid in bins do not mean much.)

In a follow up PR, I'll even make the default band different between bin and timeUnit.

@kanitw kanitw changed the title feat!: make timeunit behave more like bin feat: make timeunit for bar/rect behaves more like bin Jul 30, 2019
@kanitw kanitw merged commit 539299d into master Jul 30, 2019
@kanitw kanitw deleted the kw/timeunit branch July 30, 2019 03:52
@hoclun-rigsep
Copy link

New user, trying to follow the discussion here. With a known domain resolution of e.g. yearmonth, wouldn't you want the tick mark of the x-axis to be centered under the bar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Critical -- to fix ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve timeUnit positioning
4 participants