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

Fixed the issue with wrong width in log scale for Bar(s) #3138

Closed
wants to merge 2 commits into from

Conversation

thuiop
Copy link
Contributor

@thuiop thuiop commented Nov 11, 2022

Fixes the issue #2907, but in a hacky way. The core issue is that currently, the bars' positions relative to each other are defined by a coordinate (say x) and their width. When applying a transform, what happens is that x is transformed (into transformed_x), and the new width is obtained with transformed_width = transform(x+width/2)-transform(x-width/2). While this is actually correct, it falls apart when passing the coordinates to the matplotlib API, which asks for the bottom left of the rectangle. It is currently obtained by doing transformed_x-transformed_width/2, whereas it should really be transform(x-width/2), which is obviously not the same in the case of the log.
In this PR, I solve the problem by storing the transform(x-width/2) value and retrieving it when needed, but this feels a bit weird to have it as an additional variable ; however we cannot ditch the normal transformed_x as it is needed for other things than the bars. There is probably some underlying flow change that would make this more straightforward but I expect this to be a big change.

@mwaskom
Copy link
Owner

mwaskom commented Nov 11, 2022

Is this necessary, since Mark._plot has access to the scales?

@thuiop
Copy link
Contributor Author

thuiop commented Nov 11, 2022

Hm. Then it may be possible to do it in a better way, but I fear it will still feel weird ; in the _unscale_coords function, where the magic happens, it is the axis.get_transform().inverted() which is used (which ultimately comes from the scales, I agree). Perhaps the best way to do it would be to get rid of the _unscale_coords function in Plot altogether and do the relevant conversions in the Mark directly, but this is quite a larger step.

@mwaskom
Copy link
Owner

mwaskom commented Nov 11, 2022

Perhaps the best way to do it would be to get rid of the _unscale_coords function in Plot altogether and do the relevant conversions in the Mark directly

That seems like it would be unnecessarily repetitive in most cases?

@thuiop
Copy link
Contributor Author

thuiop commented Nov 11, 2022

Is this necessary, since Mark._plot has access to the scales?

Actually, while looking at it, I would say it is, because in Mark._plot data already contains the transformed x and width and not the originals. It is surely possible to retrieve the original x and width with the scales but this would probably be uglier than the solution I proposed.

@thuiop thuiop closed this Jan 23, 2023
@thuiop thuiop deleted the fix-width-log branch January 23, 2023 08:19
@thuiop
Copy link
Contributor Author

thuiop commented Jan 23, 2023

(superseded by #3217)

@mwaskom
Copy link
Owner

mwaskom commented Jan 23, 2023

Thanks for tracking down the root cause!

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

2 participants