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

Refine logarithmic scaling / tick generation #9166

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 26, 2021

Fix: #7332

This changes the log scale to generate ticks based on the data range. Also generates more ticks (3, 15).

Random example data: [300, 2000, 10000, 800000, 80, 200000, 40]:
(master)
image

(pr)
image

Another: data: [30070000, 30008000, 30400000, 31000000, 30020000, 30000900, 30000700]
(master)
image

(pr)
image

data: [80000, 3008000, 3400000, 3100000, 3020000, 3000900, 6000000], autoSkip: false
(master)
image

(pr)
image

data: [31.16, 31.46, 32.02, 30.08, 29.47, 27.46, 29.05, 29.09, 31.09, 28.41]
(master)
image

(pr)
image

@kurkle kurkle marked this pull request as ready for review May 26, 2021 18:26
@kurkle kurkle requested review from benmccann and etimberg May 26, 2021 18:26
@kurkle
Copy link
Member Author

kurkle commented May 26, 2021

Filename Size Change
dist/chart.esm.js 70.6 kB +254 B (0%)
dist/chart.js 89.7 kB +254 B (0%)
dist/chart.min.js 63.9 kB +167 B (0%)

@benmccann
Copy link
Contributor

The screenshots for the PR definitely look better than the screenshots for master!

I had a branch to fix this here: https://github.com/benmccann/Chart.js/tree/log-scale-ticks. I'd be curious how it compares on these datasets. There are a couple things in the screenshots above that I think still have a bit of room for improvement. I'd gotten things working pretty well, but never had time to update all the tests to match the new implementation

@kurkle
Copy link
Member Author

kurkle commented May 26, 2021

@benmccann my implementation was initially very similar to yours, excluding those +/-5% adjustments that probably do not work well in all cases.

I agree there is still room for improvement.

This might not be desirable behavior (generating 10 ticks between 27 and 28):
image

etimberg
etimberg previously approved these changes May 27, 2021
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Code and screenshots look fine. Concerned that this might be too breaking

@kurkle
Copy link
Member Author

kurkle commented May 27, 2021

Concerned that this might be too breaking

I share the concern. Maybe this should be delayed to v4.

@benmccann
Copy link
Contributor

benmccann commented May 27, 2021

This PR does change the UI a fair amount, but I'm not really sure I'd consider it breaking since it doesn't change the API at all

@kurkle
Copy link
Member Author

kurkle commented Aug 22, 2022

@etimberg @LeeLenaleee @benmccann how about merging this?

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.

log scale step size too large
4 participants