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

Fix data in financial sample #6244

Merged
merged 7 commits into from
May 15, 2019
Merged

Conversation

benmccann
Copy link
Contributor

I noticed while manually testing autoskip improvements that the financial sample is skipping 1993 and 1994 when unit is set to year because my code for generating sample dates was not too accurate.

etimberg
etimberg previously approved these changes May 4, 2019
kurkle
kurkle previously approved these changes May 5, 2019
@nagix
Copy link
Contributor

nagix commented May 6, 2019

When unit is 'hour', a wrong time series will be generated (A gap between Jan 08 and 15):

Array (60)
0 {t: "Mon Jan 08 1990 09:30:00 GMT+0800", y: "27.80"}
1 {t: "Mon Jan 08 1990 10:00:00 GMT+0800", y: "28.69"}
2 {t: "Mon Jan 08 1990 11:00:00 GMT+0800", y: "30.47"}
3 {t: "Mon Jan 08 1990 12:00:00 GMT+0800", y: "30.50"}
4 {t: "Mon Jan 08 1990 13:00:00 GMT+0800", y: "31.12"}
5 {t: "Mon Jan 08 1990 14:00:00 GMT+0800", y: "28.83"}
6 {t: "Mon Jan 08 1990 15:00:00 GMT+0800", y: "29.58"}
7 {t: "Mon Jan 08 1990 16:00:00 GMT+0800", y: "30.39"}
8 {t: "Mon Jan 15 1990 09:30:00 GMT+0800", y: "30.07"}
9 {t: "Mon Jan 15 1990 10:00:00 GMT+0800", y: "31.86"}
...

@benmccann
Copy link
Contributor Author

Thanks for catching that @nagix. I've pushed a fix

@nagix
Copy link
Contributor

nagix commented May 6, 2019

The array starts from Jan 2. I think the time between 0:00 and 9:30 needs to be skipped to 9:30 on the same day.

Array (60)
0 {t: "Tue Jan 02 1990 09:30:00 GMT+0800", y: "30.51"}
1 {t: "Tue Jan 02 1990 10:00:00 GMT+0800", y: "30.10"}
2 {t: "Tue Jan 02 1990 11:00:00 GMT+0800", y: "27.88"}
3 {t: "Tue Jan 02 1990 12:00:00 GMT+0800", y: "27.72"}
4 {t: "Tue Jan 02 1990 13:00:00 GMT+0800", y: "27.91"}
5 {t: "Tue Jan 02 1990 14:00:00 GMT+0800", y: "28.04"}
6 {t: "Tue Jan 02 1990 15:00:00 GMT+0800", y: "27.72"}
7 {t: "Tue Jan 02 1990 16:00:00 GMT+0800", y: "27.56"}
8 {t: "Wed Jan 03 1990 09:30:00 GMT+0800", y: "26.93"}
9 {t: "Wed Jan 03 1990 10:00:00 GMT+0800", y: "27.69"}
...

@benmccann
Copy link
Contributor Author

Thanks. Updated

Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

It is still incorrect when unit is 'day' (see Jan 6 and 7).

Array (60)
0 {t: "Mon Jan 01 1990 00:00:00 GMT+0800", y: "28.81"}
1 {t: "Tue Jan 02 1990 00:00:00 GMT+0800", y: "28.93"}
2 {t: "Wed Jan 03 1990 00:00:00 GMT+0800", y: "28.67"}
3 {t: "Thu Jan 04 1990 00:00:00 GMT+0800", y: "27.43"}
4 {t: "Fri Jan 05 1990 00:00:00 GMT+0800", y: "27.28"}
5 {t: "Sat Jan 06 1990 00:00:00 GMT+0800", y: "27.17"}
6 {t: "Sun Jan 07 1990 00:00:00 GMT+0800", y: "28.99"}
7 {t: "Mon Jan 08 1990 00:00:00 GMT+0800", y: "30.38"}
8 {t: "Tue Jan 09 1990 00:00:00 GMT+0800", y: "30.35"}
9 {t: "Wed Jan 10 1990 00:00:00 GMT+0800", y: "30.27"}
...

samples/scales/time/financial.html Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

Yikes. I should have had a coffee yesterday. Thanks for the thorough testing. I've updated it as suggested

nagix
nagix previously approved these changes May 9, 2019
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

LGTM

kurkle
kurkle previously approved these changes May 9, 2019
@simonbrunel
Copy link
Member

Do we really want to keep that sample in the core repository since now we have the chartjs-chart-financial project? Or maybe we should rename it for something more generic.

@benmccann
Copy link
Contributor Author

On https://www.chartjs.org/samples/latest/ the link says "Time Series". I'm fine to rename the file to be timeseries.html instead of financial.html. I'd like to keep this sample in this repo though because I think it's nice to have a realistic time series chart as a sample and this is currently our only time series sample.

@simonbrunel
Copy link
Member

+1 for timeseries.html. My comment was more about not confusing the user who may think that there is no official solution to handle financial charts. I did the same for the datalabels plugin, i.e. removed the associated example since we got many support tickets about user custom implementation.

@benmccann
Copy link
Contributor Author

The other thing we could do is place a note on the sample like:

This example demonstrates a time series scale by drawing a financial line chart using just the core library. For more specific functionality for financial charts, please see chartjs-chart-financial

If we add the note, I wonder if we'd still want to rename? The thing I like about the current file name is that it gives us a way to introduce additional time series samples. If it were named timeseries.html then it might be confusing to differentiate the samples. The note might better address the potential user confusion

@simonbrunel
Copy link
Member

I like it

@benmccann benmccann dismissed stale reviews from kurkle and nagix via 8a971d2 May 9, 2019 14:33
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants