Rectangle patch angle attribute and patch __str__ improvements by cdeil · Pull Request #7536 · matplotlib/matplotlib

cdeil · 2016-12-01T08:54:04Z

We're in the process of writing an astropy.regions package for astronomers, and we're adding an as_patch method to convert regions to matplotlib patch objects for plotting.

While writing a test for the rectangle region / patch, I noticed that matplotlib.patches.Rectangle._angle that was added in #1405 isn't exposed as a public attribute?

If that's the case, would it be possible to expose it via an angle attribute or get_angle() method?

I noticed that other parameters are exposed as a wild mix of attributes and get_ methods:

    def test_as_patch(self):
        patch = self.reg.as_patch()
        assert_allclose(patch.xy, (3, 4))
        assert_allclose(patch.get_width(), 4)
        assert_allclose(patch.get_height(), 3)
        assert_allclose(patch._angle, 5)

Presumably there are backward-compat constraints or name collisions, that make exposing patch data members in a uniform way difficult. Is there something that can be improved in general, or for Rectangle.angle specifically?

dopplershift · 2016-11-30T17:33:50Z

The usage of _angle inside Rectangle is simple enough (just one spot). Given that it's a constructor parameter, I don't see any reason not to just rename _angle to angle. Would you submit a PR with that?

cdeil · 2016-12-01T09:00:38Z

@dopplershift - I've attached a commit with the change you suggest.

While looking at the file I noticed that Rectangle.__str__ and Ellipse.__str__ don't print the angle. They should, right?
Do you want me to fix this here in this PR also?

I see that you don't have unit tests with asserts on simple data members like angle or __str__.
Do you want me to add something, or do you prefer to only maintain tests for non-trivial things, to keep it lean?

codecov-io · 2016-12-01T10:10:05Z

Current coverage is 61.88% (diff: 100%)

Merging #7536 into master will not change coverage

@@             master      #7536   diff @@
==========================================
  Files           173        173          
  Lines         56140      56140          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          34743      34743          
  Misses        21397      21397          
  Partials          0          0

Powered by Codecov. Last update 45e4a46...cc8c959

dopplershift · 2016-12-01T17:53:48Z

Thanks! If you're willing to do the work, I'd say yes to both of your questions; I'm certainly not going to say no to more tests.

cdeil · 2016-12-01T18:03:56Z

OK, I'll do it here.

Side comment: when I try to run the tests following your docs, I get this:

$ python tests.py 
Python byte-compilation optimization level: 0
Traceback (most recent call last):
  File "tests.py", line 46, in <module>
    success = test(argv=sys.argv[0:1] + extra_args, switch_backend_warn=False)
TypeError: test() got an unexpected keyword argument 'argv'

I presume that's because it's picking up the MPL 1.5 from my site-packages, because I didn't install the dev version?
That's pretty error prone.
I think I saw you're changing to pytest ... +10 to that and to get rid of such a test.py file that doesn't say which MPL version was picked up or what the problem was.

cdeil · 2016-12-01T18:56:05Z

@dopplershift - Please review.

dopplershift

Looks good overall to me. Just the minor pep8 nit that the tests picked up.

dopplershift · 2016-12-01T20:20:37Z

lib/matplotlib/patches.py

    def __str__(self):
-        return "Ellipse(%s,%s;%sx%s)" % (self.center[0], self.center[1],
-                                         self.width, self.height)
+        pars = self.center[0], self.center[1], self.width, self.height, self.angle


Can you wrap this line so it's less than 80 columns?

I've amended the last commit to wrap to 80 char here.
And I've thrown in a wedge, arc and test docstring.
Hope you like it.

dopplershift

Thanks for the other additions!

Unfortunately, you added a new > length 80 line.

dopplershift · 2016-12-02T00:09:41Z

lib/matplotlib/tests/test_patches.py

+    assert str(p) == 'Wedge(center=(1, 2), r=3, theta1=4, theta2=5, width=6)'
+
+    p = mpatches.Arc(xy=(1, 2), width=3, height=4, angle=5, theta1=6, theta2=7)
+    assert str(p) == 'Arc(xy=(1, 2), width=3, height=4, angle=5, theta1=6, theta2=7)'


Unfortunately, now this line is too long.

I've amended my commit to fix this PEP8 and checked PEP8 locally. Should be OK now.

dopplershift · 2016-12-05T22:59:30Z

Looks like just a freak windows failure (one job, unrelated). Merging...

Thanks!

cdeil mentioned this pull request Nov 30, 2016

Improve shape classes and MPL plotting astropy/regions#85

Merged

9 tasks

dopplershift added the status: needs patch label Nov 30, 2016

Change Rectangle._angle to Rectangle.angle

cc8c959

dopplershift removed the status: needs patch label Dec 1, 2016

dopplershift requested changes Dec 1, 2016

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Dec 1, 2016

cdeil force-pushed the issue-7536 branch 2 times, most recently from 067734c to d818e49 Compare December 1, 2016 20:47

cdeil changed the title ~~Expose Rectangle patch angle attribute~~ Rectangle patch angle attribute and patch __str__ improvements Dec 1, 2016

dopplershift requested changes Dec 2, 2016

View reviewed changes

Improve patch __str__ and add test

02bde21

cdeil force-pushed the issue-7536 branch from d818e49 to 02bde21 Compare December 5, 2016 08:13

dopplershift approved these changes Dec 5, 2016

View reviewed changes

dopplershift merged commit 83a3716 into matplotlib:master Dec 5, 2016

moorepants mentioned this pull request Sep 28, 2017

Make Rectangle._angle public. #9234

Closed

Uh oh!

Conversation

cdeil commented Dec 1, 2016

Uh oh!

dopplershift commented Nov 30, 2016

Uh oh!

cdeil commented Dec 1, 2016

Uh oh!

codecov-io commented Dec 1, 2016

Current coverage is 61.88% (diff: 100%)

Uh oh!

dopplershift commented Dec 1, 2016

Uh oh!

cdeil commented Dec 1, 2016

Uh oh!

cdeil commented Dec 1, 2016

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cdeil Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

cdeil Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

dopplershift commented Dec 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants