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: Add "auto" unit to timeunit transform #3645

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paulrosenzweig
Copy link

This pull request adds a new "auto" unit option to the TimeUnit transform. When set to "auto", the transform will automatically choose the appropriate time unit based on the input data.

I wanted to try and solve the problem discussed in #1310: plotting discretized timeseries data without knowing the grain.

Here's a demo Observable notebook. It patches the timeunit transform and uses DuckDB to produce differently grouped time data.

Problem/Solution

Currently, you can discretize data into months using this transform.

{"type": "timeunit", "field": "date", "units": ["year", "month"]}

But, if your data is already grouped by month, there's no way to infer that from the data.
This PR adds an "auto" unit option that would work with this example data.

[{"date": "2023-01-01"}, {"date": "2023-02-01"}, {"date": "2023-03-01"}]
{"type": "timeunit", "field": "date", "units": ["auto"]}

Alternate approaches

In an issue comment, @jheer suggested creating a new discretizing time scale that acts like a band scale. I believe that discussion happened before the TimeUnit transform was added. Enhancing TimeUnit seemed more natural fit than adding a scale, but I'd love to hear any other thoughts! I meant this PR mostly as a discussion starter with some running code.

Questions

  • Is the API right?
    I'm uncertain about the "units": ["auto"] parameter. Would a separate param be better? e.g. "inferUnits": true or similar?
  • Should I pull in the dependency?
    I created a small npm package that does the work to determine the likely unit/step. It felt like a separate concern that's not specific to vega, but it's very small. Should I pull that into vega-time as part of this PR?
    The main reason to leave it separate IMO is to add tests that would be annoying to have in Vega. I want to run the tests with Node booted into different timezones, but that's annoying requirement to put on a bigger project.

@paulrosenzweig paulrosenzweig changed the title Add "auto" unit to timeunit transform feat: Add "auto" unit to timeunit transform Jan 12, 2023
@domoritz domoritz requested a review from jheer January 13, 2023 16:04
@kanitw kanitw requested a review from a team as a code owner May 20, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant