-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
faceted plots #527
faceted plots #527
Conversation
@shoyer Here's what we currently have:
Right now this assumes that the function for map_dataarray has the same signature as 2d xray plotting methods. |
Here's what a 4d array looks like.
|
@clarkfitzg - These are looking really great. I think this is going to be a really useful tool. The colorbar label is too close to the colorbar tick label, are we manually placing that or is this a matplotlib thing? |
Thanks! The colorbar label spacing is a matplotlib thing- it only became a problem after rotating it to match the row labels. So I expect that I can fix it by changing the anchor point or placing it manually. |
#plt.title('{coord} = {val}'.format(coord=self.col, | ||
# val=str(name)[:10])) | ||
else: | ||
# Looping over the indices helps keep sanity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can write something like the facet_data
method from Seaborn's FacetGrid? That seems like a nice separation of the iteration logic from the plotting logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's on my todo list- I better post that list here.
For this PR:
EDIT - going to revisit
On this PR or possibly a different one:
Different PR:
|
''' | ||
|
||
def __init__(self, darray, col=None, row=None, col_wrap=None, | ||
aspect=1, size=3, margin_titles=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor point, but take a look at the PEP8 indentation guidelines:
https://www.python.org/dev/peps/pep-0008/#indentation
This is an example of the first "No" case :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just ran this through pep8
command line utility- lots of violations. Fixed.
|
||
self.set_titles() | ||
|
||
def __iter__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My inclination is to not add this convenience shortcut, for two reasons:
- It's a departure from Seaborn, so it's something new for users to learn.
- It's not really necessary -- iterating over
grid.axes.flat
is much more explicit (generally a good thing), and only slightly more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this with the idea of using an abc with this class: https://docs.python.org/2/library/collections.html#collections-abstract-base-classes.
Reviewing the available abc's, I don't think we stand to gain any benefit from them. At the same time, it is conceptually a 2d array. I suppose that structure is captured well enough in the axes
and name_dicts
.
""" | ||
Parameters | ||
---------- | ||
darray : DataArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this parameter data
, like Seaborn. Eventually we might support dataset objects, too.
@shoyer Latest commit also added a little more to the docs on how to use the attributes |
self.col = col | ||
self.col_wrap = col_wrap | ||
|
||
# Next the private variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are all duplicated between private versions (below) and public versions (above). For now, let's stick with the conservative choice of keeping them private like seaborn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
@@ -45,16 +48,18 @@ Imports | |||
# Use defaults so we don't get gridlines in generated docs | |||
import matplotlib as mpl | |||
mpl.rcdefaults() | |||
mpl.rcParams.update({'figure.autolayout': True}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this doing? I think nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is undoing the seaborn import? If it doesn't seem to be changing the doc build, take it out.
@@ -411,3 +411,15 @@ Plotting | |||
plot.imshow | |||
plot.line | |||
plot.pcolormesh | |||
plot.FacetGrid | |||
|
|||
FacetGrid methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more suggestion: clarkfitzg#2
we can remove this methods and let them just show up on the doc page for FacetGrid.
I think this is pretty much good to go. Merge at your discretion! |
don't show FacetGrid methods directly in API docs
Ok, going to merge once the build passes. Will continue with the rest of the stuff next week. @shoyer should I rebase or squash these commits? |
I haven't been very careful about insisting on a clean git history so far, so don't worry about rebase/squash. |
Opening this to provide visibility as I work on this PR.
Currently this has
x
andy
string arguments for 2d plot working, but everything else still needs lots of work. I'll let people know when it's ready for review.