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

Improve time scale ticks behavior #38911

Merged

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented Feb 19, 2024

Closes

Description

The default ECharts tick selection behavior was confusing on weekly and quarterly data because the ticks did not match the actual data:
Screenshot 2024-02-16 at 12 41 51 AM (3)

This PR fixes these cases and overall improves time scale ticks behavior.

Handle weeks and quarters

For these units we force ECharts to render ticks with smaller granularity and then in the formatter function we select desired ticks only.
Weeks: we select daily ticks that have the same day of week as the underlying data.
Quarters: we select monthly ticks that are start of quarters.

Handle long durations

To render long durations relatively to the unit we use the largest possible unit for ticks that exists 3 times within the data which guarantees at least two ticks on the chart.
For example, given the dataset has weekly data but the range of the dataset is 4 years. If we render weekly ticks, they will have some random values that will match start of some weeks of the data. It will be harder to comprehend than just evenly spaced yearly ticks.

The example below demonstrates why it is needed. It has weekly data over a few years.

Without switching to a larger unit for ticks
Screenshot 2024-02-22 at 7 03 46 PM

After switching to a larger unit for ticks
Screenshot 2024-02-22 at 7 02 23 PM

@alxnddr alxnddr self-assigned this Feb 19, 2024
@metabase-bot metabase-bot bot added the .Team/DashViz Dashboard and Viz team label Feb 19, 2024
Copy link

🚨 Visual Regression Tests failed. Get the report artifact

Copy link

🚨 Visual Regression Tests failed. Get the report artifact

@alxnddr alxnddr force-pushed the force-echarts-show-ticks-only-for-existing-in-dataset-x-values branch from 8eab0b4 to 7b8b778 Compare February 20, 2024 04:46
Copy link

🚨 Visual Regression Tests failed. Get the report artifact

Copy link

🚨 Visual Regression Tests failed. Get the report artifact

@alxnddr alxnddr force-pushed the force-echarts-show-ticks-only-for-existing-in-dataset-x-values branch 2 times, most recently from 7310730 to 1d57fb1 Compare February 22, 2024 21:18
Copy link

🚨 Visual Regression Tests failed. Get the report artifact

@alxnddr alxnddr changed the title force echarts to show x-axis ticks that exist in data Improve time scale ticks behavior Feb 22, 2024
Copy link

Snapshots have been updated

{ interval: "year", count: 50, testFn: d => d.year() % 50 }, // (20) 50 year
{ interval: "year", count: 100, testFn: d => d.year() % 100 }, // (21) 100 year
export const TIMESERIES_INTERVALS = [
{ unit: "ms", count: 1, testFn: d => 0 }, // (0) millisecond
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 removed a few old dc.js files with dead code since they relied on this objects with the old name

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to a .ts file in the echarts directory

@alxnddr alxnddr marked this pull request as ready for review February 22, 2024 22:10
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label Feb 22, 2024
Copy link

Snapshots have been updated

Copy link

🚨 Visual Regression Tests failed. Get the report artifact

Copy link

replay-io bot commented Feb 22, 2024

Status Complete ↗︎
Commit 5274b3a
Results
180 Failed
⚠️ 1 Flaky
2036 Passed

Copy link
Contributor

@EmmadUsmani EmmadUsmani left a comment

Choose a reason for hiding this comment

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

Timeseries waterfall charts are no longer working in app (but they do in static viz), for example http://localhost:3000/question/133-waterfall-timeseries-uk-car-accidents

Screenshot 2024-02-23 at 8 57 40 AM Screenshot 2024-02-23 at 8 58 34 AM

@EmmadUsmani
Copy link
Contributor

Also, when we bucket by "week of year" it displays both the first and the last week as "1st"

week of year 1st twice

We can tackle this in another PR if it's not related, just make sure to file an issue

@alxnddr
Copy link
Member Author

alxnddr commented Feb 26, 2024

@EmmadUsmani can you please share more details about the waterfall question that is not working? I don't use this database due to huge delays which makes it unusable for me locally

Copy link

🚨 Visual Regression Tests failed. Get the report artifact

Copy link

Snapshots have been updated

@EmmadUsmani
Copy link
Contributor

EmmadUsmani commented Feb 28, 2024

In a lot of cases just the first tick is missing, which looks wrong from a user perspective. For example,

Screenshot 2024-02-28 at 9 17 24 AM Screenshot 2024-02-28 at 9 17 35 AM Screenshot 2024-02-28 at 9 19 04 AM

Also, I'm not really sure what's going on in this case. We went from displaying Feb 16th twice to Feb 27th twice. Is Feb 27th the correct date?

Screenshot 2024-02-28 at 9 18 31 AM

Copy link
Contributor

@EmmadUsmani EmmadUsmani left a comment

Choose a reason for hiding this comment

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

Let's resolve the above issue regarding the first date, I think it's a big regression.

@alxnddr
Copy link
Member Author

alxnddr commented Feb 28, 2024

@EmmadUsmani there is no actual regression in storybook or the app but Loki for some reason flakes and drop them, I'll investigate:
Screenshot 2024-02-28 at 2 53 43 PM
Screenshot 2024-02-28 at 2 54 01 PM

Also, I'm not really sure what's going on in this case. We went from displaying Feb 16th twice to Feb 27th twice. Is Feb 27th the correct date?

I think this spec has relative date time x-axis but due to formatting issues it includes current date which is wrong. I'll check & fix, maybe in a separate PR since this is related to actual time scale x-axis

{ interval: "year", count: 50, testFn: d => d.year() % 50 }, // (20) 50 year
{ interval: "year", count: 100, testFn: d => d.year() % 100 }, // (21) 100 year
export const TIMESERIES_INTERVALS = [
{ unit: "ms", count: 1, testFn: d => 0 }, // (0) millisecond
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to a .ts file in the echarts directory

@EmmadUsmani
Copy link
Contributor

EmmadUsmani commented Feb 28, 2024

Screenshot 2024-02-28 at 10 43 38 AM

I'm seeing this bug on waterfall charts with the total disabled. It only happens in this case, not on all waterfall charts without totals.

@alxnddr alxnddr force-pushed the force-echarts-show-ticks-only-for-existing-in-dataset-x-values branch from 30ba05b to 12b1e51 Compare February 29, 2024 18:13
@alxnddr alxnddr force-pushed the force-echarts-show-ticks-only-for-existing-in-dataset-x-values branch from 12b1e51 to 5f5c06f Compare February 29, 2024 18:36
@alxnddr
Copy link
Member Author

alxnddr commented Feb 29, 2024

@EmmadUsmani the issue with missing ticks is caused by timezones. Since this PR has grown a lot, I moved the timezone issue in a separate task #39322

@alxnddr
Copy link
Member Author

alxnddr commented Feb 29, 2024

I'm seeing this bug on waterfall charts with the total disabled. It only happens in this case, not on all waterfall charts without totals.

Fixed this, thanks

@alxnddr alxnddr merged commit 4e4c662 into echarts Feb 29, 2024
73 of 107 checks passed
@alxnddr alxnddr deleted the force-echarts-show-ticks-only-for-existing-in-dataset-x-values branch February 29, 2024 20:28
alxnddr added a commit that referenced this pull request Mar 31, 2024
* handle time series ticks in ECharts

* fix waterfall total tick

* cleanup

* specs

* Update Loki Snapshots

* review

* compensate ticks timezone

* update snapshots

* spec

---------

Co-authored-by: Metabase bot <metabase-bot@metabase.com>
alxnddr added a commit that referenced this pull request Apr 15, 2024
* handle time series ticks in ECharts

* fix waterfall total tick

* cleanup

* specs

* Update Loki Snapshots

* review

* compensate ticks timezone

* update snapshots

* spec

---------

Co-authored-by: Metabase bot <metabase-bot@metabase.com>
alxnddr added a commit that referenced this pull request Apr 18, 2024
* handle time series ticks in ECharts

* fix waterfall total tick

* cleanup

* specs

* Update Loki Snapshots

* review

* compensate ticks timezone

* update snapshots

* spec

---------

Co-authored-by: Metabase bot <metabase-bot@metabase.com>
alxnddr added a commit that referenced this pull request Apr 19, 2024
* handle time series ticks in ECharts

* fix waterfall total tick

* cleanup

* specs

* Update Loki Snapshots

* review

* compensate ticks timezone

* update snapshots

* spec

---------

Co-authored-by: Metabase bot <metabase-bot@metabase.com>
alxnddr added a commit that referenced this pull request Apr 25, 2024
* handle time series ticks in ECharts

* fix waterfall total tick

* cleanup

* specs

* Update Loki Snapshots

* review

* compensate ticks timezone

* update snapshots

* spec

---------

Co-authored-by: Metabase bot <metabase-bot@metabase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loki-update .Team/DashViz Dashboard and Viz team visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants