-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
scatter plot and hexbin plot lose x-axis when colorbar is included. #20446
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
Conversation
Thanks, let us know when you're ready for us to take a look. How does this work with How does this work with user-specified axes via |
Yes, it's already broken. Please feel free to take a look:
|
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 would say make a new issue for subplots, and ignore it for now.
As for testing, how about we make two plots, one with c
and one without, and assert that the tick labels are the same?
fig, ax = plt.supblots()
df.plot.scatter(x='x', y='y', ax=ax)
fig, ax2 = plt.subplots()
df.plot.scatter(x='x', y='y', c='c', ax=ax2)
assert ax.get_xticklabels() == ax2.get_xticklabels()
or something like that.
pandas/plotting/_core.py
Outdated
@@ -870,6 +870,8 @@ def _make_plot(self): | |||
scatter = ax.scatter(data[x].values, data[y].values, c=c_values, | |||
label=label, cmap=cmap, **self.kwds) | |||
if cb: | |||
if PATCH_MODE: |
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 assume this ``PATCH_MODE` is just for debugging?
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.
yes, it should be removed. Didn't know of a better way to do this.
pandas/plotting/_tools.py
Outdated
@@ -298,20 +298,20 @@ def _remove_labels_from_axis(axis): | |||
|
|||
def _handle_shared_axes(axarr, nplots, naxes, nrows, ncols, sharex, sharey): | |||
if nplots > 1: | |||
|
|||
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 whitespace will likely fail the linter
pandas/plotting/_tools.py
Outdated
if nrows > 1: | ||
try: | ||
# first find out the ax layout, | ||
# so that we can correctly handle 'gaps" | ||
layout = np.zeros((nrows + 1, ncols + 1), dtype=np.bool) | ||
for ax in axarr: | ||
layout[ax.rowNum, ax.colNum] = ax.get_visible() | ||
|
||
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.
Likewise
Hello @javadnoorb! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on July 04, 2018 at 03:04 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #20446 +/- ##
==========================================
+ Coverage 91.9% 91.92% +0.02%
==========================================
Files 154 158 +4
Lines 49657 49705 +48
==========================================
+ Hits 45638 45693 +55
+ Misses 4019 4012 -7
Continue to review full report at Codecov.
|
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.
removed the whitespaces
Don't worry about subplots. It would be something like |
This works:
but regardless of I think the issue is not with the xticks attribute but rather showing them. |
Trying a few of these. Quite bizarre:
|
@javadnoorb how's this going? For this PR I would ignore user-provided axes, and only worry about the common case of |
@TomAugspurger I haven't worked on this for a few days. Let me see if I can come up with a reasonable test. pandas/pandas/plotting/_tools.py Line 282 in 8a58303
pandas/pandas/plotting/_tools.py Line 296 in 8a58303
pandas/pandas/plotting/_tools.py Line 293 in 8a58303
my guess is that something like:
and
and
or something like that, should work for the test. Let me give it a shot. |
Sounds good. Let me know if that doesn't work and I can take a look.
…On Wed, Mar 28, 2018 at 11:48 AM, Javad Noorbakhsh ***@***.*** > wrote:
@TomAugspurger <https://github.com/TomAugspurger> I haven't worked on
this for a few days. Let me see if I can come up with a reasonable test.
get_xticklabels returns an object, so I think we can't use it for
comparison. I think the reason the x-labels disappear should be because of
these two lines:
https://github.com/pandas-dev/pandas/blob/8a5830305d2d9a087fbe46bd968218
104ffdfc49/pandas/plotting/_tools.py#L282
https://github.com/pandas-dev/pandas/blob/8a5830305d2d9a087fbe46bd968218
104ffdfc49/pandas/plotting/_tools.py#L296
my guess is that something like:
assert ax1.get_majorticklabels().get_visible()==ax2.get_
majorticklabels().get_visible()
and
assert ax1.get_label().get_visible()==ax2.get_label().get_visible()
or something like that, should work for the test.
Let me give it a shot.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20446 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIkLc6e7wel8eSgSbmlptCfR_HWFaks5ti77NgaJpZM4S2XhP>
.
|
Ok, I wrote a test that seems to work:
If that seems fine I'll include the changes and remove the PATCH_MODE flag. I'm not familiar with writing tests. Is this where it goes? |
That test seems sensible. Tests can go in |
removing PATH_MODE debugging flag
Ok, great. I'll check if it passes the regular checks before including the test. |
remove whitespaces
Also need a test for hexbin. This one seems to work:
|
These changes seem pretty benign. Is there any specific reason for Travis-CI to throw an error? |
pandas/plotting/_tools.py
Outdated
@@ -311,7 +311,7 @@ def _handle_shared_axes(axarr, nplots, naxes, nrows, ncols, sharex, sharey): | |||
# only the last row of subplots should get x labels -> all | |||
# other off layout handles the case that the subplot is | |||
# the last in the column, because below is no subplot/gap. | |||
if not layout[ax.rowNum + 1, ax.colNum]: | |||
if not layout[ax.rowNum + 1, ax.colNum] or getattr(ax, '_pandas_colorbar_axes', False): |
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 line is too long: https://travis-ci.org/pandas-dev/pandas/jobs/359516939#L3006
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.
Thanks. I'll fix it. Sorry I'm pretty new to the open-source world.
…s not exclude x-axis label and tick-labels
@TomAugspurger. Seems like the issue is because in the inline backend floats corresponding to location of axes have too many arbitrary trailing digits. For example two axes that have their bottom left corners at 0.3 may appear as 0.3 and 0.30000001. I rounded them in This new commit works for single plots with pdf and inline backends. It still fails for multiplots. We'll need to dig deeper for that. But it seems like we don't need the previous hack anymore. |
pandas/plotting/_core.py
Outdated
# `get_points()` have too many ad hoc trailing | ||
# digits. Unless rounded these values won't | ||
# match in the following set operations | ||
points = np.round(points, 3) |
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 worry about this causing debugging nightmares further down the road... I'd prefer to solve this local to the problem if possible.
If not, you could perhaps include a keyword to _get_axes_layout
? I'm not sure how difficult that would be to plumb through.
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 agree. That's pretty tough to track. Might be better to avoid rounding off. Especially that 3 digit rounding is pretty arbitrary and may break in unknown ways.
I made a new commit. The changes are local now. I'm forcing colorbars to take on their relevant axes positions from the parent axes. The merge passes the tests and resolves the xlabel issue (#10611 and #10678) but not with the subplots (#20455).
* minor change in comment * added condition on inline backend * added condition on inline backend * issue with floats determining the position of axes corners in ipython * fixing axis position roundoff error locally
* minor change in comment * added condition on inline backend * added condition on inline backend * issue with floats determining the position of axes corners in ipython * fixing axis position roundoff error locally * broken merge
I just remembered about this. The last commit seemed to have passed all the checks. Is there any interest to follow up on it? |
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.
Sorry for dropping this. Can you merge in master and move the whatsnew to 0.24.0?
Looks good otherwise, so ping on green.
doc/source/whatsnew/v0.23.0.txt
Outdated
@@ -1176,7 +1176,7 @@ Plotting | |||
- Bug in formatting tick labels with ``datetime.time()`` and fractional seconds (:issue:`18478`). | |||
- :meth:`Series.plot.kde` has exposed the args ``ind`` and ``bw_method`` in the docstring (:issue:`18461`). The argument ``ind`` may now also be an integer (number of sample points). | |||
- :func:`DataFrame.plot` now supports multiple columns to the ``y`` argument (:issue:`19699`) | |||
|
|||
- Bug in :func:'DataFrame.plot.scatter' and :func:'DataFrame.plot.hexbin' caused x-axis label and ticklabels to disappear when colorbar was on in IPython inline backend (:issue:`10611` and :issue:`10678`) |
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 should be moved to 0.24.0
pandas/plotting/_core.py
Outdated
@@ -882,8 +882,13 @@ def _make_plot(self): | |||
kws = dict(ax=ax) | |||
if self.mpl_ge_1_3_1(): | |||
kws['label'] = c if c_is_column else '' | |||
self.fig.colorbar(img, **kws) | |||
|
|||
cbar = self.fig.colorbar(img, **kws) |
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 refactor this and https://github.com/pandas-dev/pandas/pull/20446/files#diff-2b118bda866f3d626ceb6b529c62cd1aR932 to a method. They're identical, right? I guess aside from **kws
vs. ax=ax
?
Thanks @TomAugspurger. I cleaned up the code. It's passing the tests. Here's what the output in jupyterlab looks like: %matplotlib inline
import numpy as np
import pandas as pd
random_array = np.random.random((1000, 3))
df = pd.DataFrame(random_array,columns=['A label','B label','C label'])
df.plot.scatter('A label', 'B label', c='C label') df.plot.hexbin('A label','B label', gridsize=12); |
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.
comments, otherwise lgtm. ping on green.
pandas/plotting/_core.py
Outdated
@@ -833,6 +833,23 @@ def _post_plot_logic(self, ax, data): | |||
ax.set_ylabel(pprint_thing(y)) | |||
ax.set_xlabel(pprint_thing(x)) | |||
|
|||
def _plot_colorbar(self, kws): |
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.
can you make ax a named arg here and so you would pass as **kwds
when you call.
pandas/plotting/_core.py
Outdated
# To deal with this, this method forces the colorbar | ||
# height to take the height of the parent axes. | ||
ax = kws['ax'] | ||
img = ax.collections[0] |
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.
is there an issue we can reference about this impl. IOW next person to look at wouldn't know where these set_position come from. so a) ref this issue in a comment and b) an Ipython refernce (if you can)
thanks @javadnoorb very nice! |
thanks @jreback! |
closes #10611
closes #10678.
The x-axis for scatter plot and hexbin plot disappears when colorbar is included. This seems to be because colorbar axis is looped through in
_handle_shared_axes
:pandas/pandas/plotting/_core.py
Line 426 in 8a58303
after discussing with @TomAugspurger (issue #10611), we decided to try adding the attribute
__is_pandas_colorbar
to colorbar axis object and skipping it during handling of shared axes.I've done some tests that seem to fix the issue. But we may need more tests: