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 overlapping auto-generated ticks #6243

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

It was reported in #6109 that the time scale sometimes generates overlapping ticks. @kurkle debugged this and determined in #6115 that the issue is because the time scale's auto-tick generation doesn't consider padding.

We'd like to take most of the intelligence out of the time scale's auto tick generation and instead generate a tick at every point and use the auto-skipper to automatically decide which ticks to display. For more details see #4612

While the solution proposed in #6115 fixes the bug it also adds more logic to the time scale which is not quite going in the direction we'd like to take things longer-term. This PR may seem a bit funny standalone because when ticks.source: 'auto' we're auto-generating ticks and then checking to see if we have too many and autoskipping some of them as well. We shouldn't really have two auto-tick logics. I have a branch on my machine locally that moves much of the time scale's auto-generation logic to the auto-skipper so that everything is contained there. The change in this PR is both necessary for that longer term solution and also in the short-term fixes the bug reported in #6109

Closes #6109 #6115

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

This change would turn on autoSkip in any scale when ticks.source='auto'.
(source is only handled by time scale of our internal scales.)

While this probably works, I have some worries:

  • I don't think its good practice to override option based on another option.
  • Using autoSkip to fix bad tick generation is even further away from fixing the actual issue than Fix overlapping auto-generated ticks on time scale #6115.
  • I think long term goal is also to move autoSkip to update. I suppose this same issue would arise even with autoSkip enabled then.

Finally, I don't think the underlying issue (width/height not being at their final values when ticks are generated) can be fixed without breaking changes.

@benmccann
Copy link
Contributor Author

Thanks for taking a look @kurkle. I'm happy to discuss more on Slack if this ends up getting too complicated for GitHub issues. But either way is fine with me

I don't think its good practice to override option based on another option.

In general, I agree. I think this case might be a bit different because we've essentially decided that we don't like the way one of these features is built so we're going to replace it and turn on the other feature, which offers essentially equivalent functionality, to maintain backwards compatibility

Using autoSkip to fix bad tick generation is even further away from fixing the actual issue than #6115.

It depends what you think the actual issue is. I'm guessing we saw different things as being the root issue. I think the issue is that tick generation has to care about the scale size. The plan we had in mind is to make tick generation very dumb and not care about scale size and to use autoskip to handle any intelligence around where to place ticks. What I'm suggesting is to start going in that direction now rather than making the tick generation care even more about the scale dimensions

I think long term goal is also to move autoSkip to update. I suppose this same issue would arise even with autoSkip enabled then.

It's something I've suggested in the past, but I'm not entirely sure whether it aligns with my current thinking, which is still evolving. Another possibility is that update just generates the ticks independent of scale size and then _autoSkip is called after the scale dimensions are calculated and handles showing only the ones that fit. The other thing that could be beneficial as well is to stop calling update twice and call fit instead. This would align very well with making update not care about scale size

I don't think the underlying issue (width/height not being at their final values when ticks are generated) can be fixed without breaking changes.

I think that making tick generation independent of width/height and then calling _autoSkip after width/height has been calculated would be both simpler than what happens now and backwards compatible

@benmccann
Copy link
Contributor Author

I've been doing some profiling and I think that the solution that we had been talking about for improving auto-skip does not scale well enough, so I'll close this for now and approve the other PR for the time being

@benmccann benmccann closed this May 7, 2019
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.

Overlapping labels on x axis
2 participants