GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||
GeezerVison is showing github.com — Honor set_rlabel_position on partial-wedge polar axes by mertkayacs · Pull Request #32539 · matplotlib/matplotlib Conversation
|
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The new manual-label branch in get_yaxis_text1_transform() chooses horizontal alignment from get_theta_direction(), but test_polar_rlabel_position_wedge only exercises the default counterclockwise direction and checks anchor positions. Could you also run it with set_theta_direction(-1) and assert the alignment or rendered label placement? That would cover the clockwise branch introduced here, including the new rlabel padding path.
Review prepared by Codex on behalf of agammann.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
With set_theta_direction(-1) and an explicit rlabel position on a wedge, the rlabel pad shift pointed to the clockwise side of the radial line while RadialTick.update_position aligns the text on the counterclockwise side, so the labels straddled the gridline. Shift toward the side the text is on. Extend test_polar_rlabel_position_wedge to both theta directions and assert the anchor, the pad shift and that the whole label stays on the padded side of the radial line.
| full = _is_full_circle_deg(thetamin, thetamax) | ||
|
|
||
| if full: | ||
| if full or axes._r_label_position_manual: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Slight logic concern: "full or manually set" seems a strange condition.
I feel there should be a cleaner way of expressing this. The point is that "full or manual" takes the _rlabel_position_transform whereas "non-manual wedge" must decide on a suitable angle based on thetamin. It's likely not possible to push that decision up so that we don't have if/else distinction here, because we evalutate on the current thetamin. So the reverse question: Could we only store the r_label angular value and create the transform here?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Keep only the rlabel angle, None until set_rlabel_position is called, in place of the Affine2D and the flag that recorded whether the position was set. PolarAxes._get_rlabel_theta decides in one place where the labels go: the set angle, else the default angle on a full circle and thetamin on a partial wedge. The lazily evaluated _RLabelTransform builds the label translation from it, and label1, tick1, the 'rlabel' pad and RadialTick.update_position all use it, so the "full or manual" special cases are gone from draw(), get_yaxis_text1_transform and update_position. With a set position on a wedge, this also fixes the padding when the position changes after a draw or when the ticks existed before it was set, keeps the end-edge labels and tick marks (label2, tick2) on their edge where they used to jump to the set angle plus one radian, and turns the start tick marks perpendicular to the label line as on the default edge. With inverted theta limits, the default labels now follow thetamin, which their alignment and padding already assumed. Add tests for setting the position over existing ticks and for a position at thetamin giving the default wedge layout.
|
That works: the only rlabel state is now the angle ( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
"set_rlabel_position" don't work when "set_thetamin"
There was an error while loading. Please reload this page.