FIX: Include (0, 0) offsets in scatter autoscaling #22476
Conversation
| bbox = transforms.Bbox.null() | ||
| bbox.update_from_data_xy(offsets) | ||
| return bbox | ||
|
|
There was a problem hiding this comment.
Should the check for has_offsets a few lines earlier also be dropped? (if offset_trf is "anything", it certainly can map (0, 0) to anywhere and therefore autoscaling based on the collection probably doesn't make sense either.
Regardless of this comment, the PR as is fixes the original issue; this is just about a more general case.
There was a problem hiding this comment.
👍 I got rid of that entire block above.
2120724 to
6af5c91
Compare
| if has_offsets and not offset_trf.contains_branch(transData): | ||
| # if there are offsets but in some coords other than data, | ||
| # then don't use them for autoscaling. | ||
| return transforms.Bbox.null() |
There was a problem hiding this comment.
I am very wary of just ripping out a block of code like this without justification. The original issue was that the autoscaling logic was faulty when the offsets was basically null. But this block is for a special case where the offset transform isn't the usual transform and that we can't usually calculate the autoscale correctly in that case. Just ripping this out doesn't address this special case.
There was a problem hiding this comment.
I am also highly suspicious that ripping this block out did not trigger a test failure
There was a problem hiding this comment.
Thanks for the push to investigate this, Ben. You were right to be wary of this passing without any tests.
I added a new test that adds a scatter in transAxes coordinates (fixing the point in Axes space). Which I think we agree should not contribute to autoscaling. When I ripped this chunk of code out, it would have contributed.
I did however change the conditional to be: "if offsets is an updated transform (not Identity), and does not contain the transData path". Rather than checking for that possibly Falsy "has_offsets".
A scatter point at (0, 0) has offsets with 0s and any(offsets) then gets evaluated to False. We still want to include this point in the autoscaling if it is there.
6af5c91 to
f93a0fc
Compare
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…-v3.5.x Backport PR #22476: FIX: Include (0, 0) offsets in scatter autoscaling
PR Summary
A scatter point at (0, 0) has offsets with 0s and
any(offsets)then gets evaluated to False. We still want to include this point
in the autoscaling if it is there.
Closes #22470
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).