Conversation
github-actions
bot
added
topic: color/color & colormaps
topic: transforms and scales
Documentation: examples
topic: mplot3d
CI: Run cibuildwheel
CI: Run cygwin
backend: pdf
GUI: Qt
GUI: tk
Documentation: build
Documentation: devdocs
Documentation: user guide
topic: collections and mappables
topic: polar
topic: pyplot API
topic: styles
topic: text
topic: ticks axis labels
topic: widgets/UI
backend: svg
Documentation: tutorials
topic: animation
topic: images
topic: sphinx extension
topic: date handling
topic: figures and subfigures
topic: markers
topic: rcparams
topic: toolbar
labels
github-actions
bot
removed
CI: Run cygwin
backend: pdf
GUI: Qt
GUI: tk
Documentation: build
Documentation: devdocs
Documentation: user guide
topic: collections and mappables
topic: pyplot API
topic: styles
topic: text
topic: ticks axis labels
topic: widgets/UI
backend: svg
Documentation: tutorials
topic: animation
topic: images
topic: sphinx extension
topic: date handling
topic: figures and subfigures
topic: markers
topic: rcparams
topic: toolbar
labels
r3kste
left a comment
r3kste
left a comment
There was a problem hiding this comment.
@Sreekanth-M8 This looks good. As discussed, you can go ahead condense this solution to put as a comment to ask for initial feedback. There is just one minor comment about radial locator.
r3kste
force-pushed
the
ChoiceLocator
branch
from
269eb94 to
dec73c7
Compare
r3kste
force-pushed
the
ChoiceLocator
branch
from
dec73c7 to
8aa5624
Compare
There was a problem hiding this comment.
I ran test_polar.py and left some comments about the failing tests.
-
For Theta Tick, there is one test failing:
lib/matplotlib/tests/test_polar.py::test_polar_theta_limits. -
For Radial Tick, there are a lot of tests failing.
As mentioned below, it would be better if we stick with only changing Theta ticks in this PR (as that is what the issue also expects).
| def __init__(self, base=None, choices=None): | ||
| if choices is None: | ||
| choices = [ | ||
| np.arange(-360, 360, 30.0), |
There was a problem hiding this comment.
Using 30 degree steps as first priority causes a lot of tests in test_polar.py to fail. It looks like the previous behavior was to use 45 degree intervals. So, it would be better to remove 30 degree intervals.
| np.arange(-360, 360, 30.0), |
There was a problem hiding this comment.
Previous behavior uses 45 degree intervals only for full circle. for other arcs with angles (< 360),
30 degree intervals are also used. I think adding a check for full circle might be better.
| def get_tick_space(self): | ||
| ends = mtransforms.Bbox.unit().transformed( | ||
| self.axes.transAxes - self.get_figure(root=False).dpi_scale_trans) | ||
| thetamin, thetamax = self.axes._realViewLim.intervalx | ||
| rmin, rmax = self.axes._realViewLim.intervaly | ||
| rorigin = self.axes.get_rorigin() | ||
| radius = min(ends.height, ends.width) * 72 | ||
| actual_ratio = rmax / (rmax - rorigin) | ||
| if abs(thetamax - thetamin) > np.pi / 2: | ||
| radius /=2 | ||
| # Having a spacing of at least 3 just looks good | ||
| size = self._get_tick_label_size('y') * 3 | ||
| if size > 0: | ||
| return int(np.floor(radius * actual_ratio / size)) | ||
| else: | ||
| return 2**31 - 1 | ||
|
|
There was a problem hiding this comment.
Changing the behavior of radial ticks causes a lot of tests to fail.
Now that I think about it, the issue doesn't really expect us to change Radial Ticks. It only mentions about the issue with Theta ticks. So I think it would be better if we stick to just Theta Ticks for this PR.
2 participants
PR summary
added ChoiceLocator as per 20012
PR checklist