Update plotting API #24
Conversation
|
The testing should probably be done as part of #20 |
|
|
||
| Parameters | ||
| ---------- | ||
| ax : `~matplolib.axes`, optional |
There was a problem hiding this comment.
This isn't actually optional, is it?
also, matplotlib
There was a problem hiding this comment.
It can be optional I think (at least it was in photutils), but then it should be defined with e.g. ax = plt.gca() when not given as parameter.
There was a problem hiding this comment.
Yes, that's what I had in mind
|
Can I merge this? |
| @@ -161,6 +161,36 @@ def to_shapely(self): | |||
| """ | |||
| raise NotImplementedError("") | |||
|
|
|||
| @abc.abstractmethod | |||
| def make_patch(self, **kwargs): | |||
There was a problem hiding this comment.
I would call this as_mpl_patch (we've used this name in another package, but I can't remember which one now!)
There was a problem hiding this comment.
Sorry, I missed that... I've commented it on it :) If we do decide to change it, it's only a search and replace at least? 😬
There was a problem hiding this comment.
👍 😄 I will change to as_patch for now, ok?
|
@joleroi - apart from the naming suggestion, looks good to me! |
| """ | ||
| import matplotlib.pyplot as plt | ||
|
|
||
| ax = plt.gca() if ax is None else ax |
There was a problem hiding this comment.
For some weird reason, this comment was collapsed:
Personnaly I prefer if/else-s to be written in multi line, rather than in this form. But either case the else statement is not needed here.
There was a problem hiding this comment.
Is there a guideline for that (I didn't find any ) http://docs.astropy.org/en/stable/development/codeguide.html
I prefer the one liners, even if they require an useless else statement
There was a problem hiding this comment.
Well, it's not written anywhere but historically there was a long argument about the support of ternary conditional operators in python. And actually none of the examples in PEP308 deal with optional agrs but list comprehensions, lambdas, etc.
Also, I cannot find any such usage of it in numpy, and astropy core (except one single case in io.fits).
There was a problem hiding this comment.
You can always use a one-liner without else: if ax is None: ax=plt.gca().
There was a problem hiding this comment.
That is also in the "Rather not" category in PEP8 (https://www.python.org/dev/peps/pep-0008/)
There was a problem hiding this comment.
Could this be usefully shortened to ax = ax or plt.gca()? It may be less readable than the ternary, though, so just thumbs-down this if you don't like it.
|
(@joleroi - It looks like you have both tabs and spaces used as indents. For most editors you can set to use spaces only for tabs as well, so it's worth opting in it). |
|
@bsipocz that's very well possible 😄 I switched to vim on monday after having used emacs for almost 10 years now (I feel like a 5 year old when typing). May the editor gods have mercy upon my soul ... |
|
Switching to vim from emacs? 😢 |
|
I would like to show you my awesome working environment that took me 4 days to set up but will make me so much more productive in the future 👍 |
|
Do you still believe in fairy tales? ;) |
|
@bsipocz Can you please have a look at appveyor? Maybe one just has to restart it but I never figured out how ... |
|
@joleroi -looks like a fluke, I've restarted it. If you login to appveyor there should be a "restart-PR" button on the top right part of the actual build. If you don't see that then Christoph needs to add you appveyor admin rights. |
|
I don't see this button. @cdeil can you give me appveyor admin rights? Ok to merge now? |
|
I don't see the restart button on Appveyor for the Probably only @astrofrog and @keflavich have such powers? |
|
AFAIK appveyor has different rights to github, e.g. I have it for regions as well as most astropy repos, though not having commit rights for most of them. |
|
@astrofrog - Thanks. Just in case others face this: I had to log out and log back in and select "Astropy account" at login to be able to get these Appveyor admin powers for Astropy repos. It still shows me logged in as Christoph Deil after though, so that is pretty confusing. Anyways: green light from appveyor for this PR now. |
|
@cdeil - Yep, that's one of the many annoying things about appveyor ;) |
|
Ah yes, @bsipocz you already have access to all repos on appveyor :) |
|
@astrofrog - 👍 all, or most, it's the same as I haven't tried to do anything with many of them anyway ;) |
This implements the plotting API as discussed in #12