bugfix for plot for string x values #18726
Conversation
pandas/plotting/_core.py
Outdated
| @@ -573,6 +573,8 @@ def _get_xticks(self, convert_period=False): | |||
| self.data = self.data[notna(self.data.index)] | |||
| self.data = self.data.sort_index() | |||
| x = self.data.index._mpl_repr() | |||
| elif all(isinstance(val, str) for val in index): | |||
There was a problem hiding this comment.
I think the general preference is to use pandas.compat.string_types for isinstance checks against strings. Looks like string_types is already imported, so just switching to isinstance(val, string_types) should be fine.
pandas/plotting/_core.py
Outdated
| @@ -573,6 +573,8 @@ def _get_xticks(self, convert_period=False): | |||
| self.data = self.data[notna(self.data.index)] | |||
| self.data = self.data.sort_index() | |||
| x = self.data.index._mpl_repr() | |||
| elif all(isinstance(val, string_types) for val in index): | |||
There was a problem hiding this comment.
This will be slow for large indexes. Try index.is_object().
There was a problem hiding this comment.
Is is_object equal to is_str? The reason for the generator is that in my example the datatype of the index was wrong (i.e. object and not str) and the generator is only slow when there are str objects. My assumption was that in the case that the index contains str, the index size is relative small.
There was a problem hiding this comment.
you can do
index.inferred_type in ['string', 'unicode'] is pretty performant
There was a problem hiding this comment.
ok, I changed the code to use index.inferred_type
|
can you add a whatsnew note in 0.22 |
|
Hello @boeddeker! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on February 11, 2018 at 11:29 Hours UTC |
|
I add a whatsnew note and a test |
pandas/tests/plotting/test_frame.py
Outdated
|
|
||
| line1, line2 = ax.lines | ||
| # xdata should not be touched (Earlier it was [0, 1]) | ||
| np.testing.assert_equal(line1.get_xdata(), x1) |
There was a problem hiding this comment.
we don't use np.testing, instead use tm.assert_numpy_array_equal
There was a problem hiding this comment.
tm.assert_numpy_array_equal does not work and tm.assert_array_equal does not exsist (The comparison is between numpy and a list).
There was a problem hiding this comment.
sure it does
turn the list into a numpy array
you can not compare a list and an array in our testing by default
pandas/tests/plotting/test_frame.py
Outdated
| ax = df2.plot(ax=ax) | ||
|
|
||
| line1, line2 = ax.lines | ||
| # xdata should not be touched (Earlier it was [0, 1]) |
There was a problem hiding this comment.
blank line before comments
doc/source/whatsnew/v0.22.0.txt
Outdated
| @@ -292,7 +292,7 @@ Plotting | |||
| ^^^^^^^^ | |||
|
|
|||
| - :func: `DataFrame.plot` now raises a ``ValueError`` when the ``x`` or ``y`` argument is improperly formed (:issue:`18671`) | |||
| - | |||
| - Bug in :func: `DataFrame.plot` where the ``xtickvalues`` are overwritten (:issue:`#18687`) | |||
| - | |||
There was a problem hiding this comment.
no space after :func:, can you add a bit more text on what a user would want to know here
issue is: (:issue:`18687`) (no #)
There was a problem hiding this comment.
The line above mine has also a whitespace after :func:
|
pls rebase |
|
Ok, I did a rebase with master. |
|
Can you add another commit with the release note? Those need to be included in the docs. It looks like this changes is failing other tests: https://travis-ci.org/pandas-dev/pandas/jobs/340101818#L2288. Do those fail for you locally? |
|
Where should I add a release note? The file where I originally added the note, has completely changed. Regarding the test, the test also fails local on my machine. _, ax = plt.subplots()
ax.plot(x, y)
ax.set_xticks([1, 2]) # -> x labels: ['', 'a', 'b']
print('xticklabels', list(ax.get_xticklabels())) # xticklabels [Text(0,0,''), Text(0,0,'')]
print('xticks', list(ax.get_xticks())) # xticks [1.0, 2.0]
_, ax = plt.subplots()
ax.plot(x, y)
ax.set_xticks(['b', 'c']) # -> x labels: ['', 'a', 'b']
print('xticklabels', list(ax.get_xticklabels())) # xticklabels [Text(0,0,''), Text(0,0,'')]
print('xticks', list(ax.get_xticks())) # xticks [1.0, 2.0] |
|
note goes in whatsnew for 0.23.0 / bugfixes / plotting |
|
According to matplotlib/matplotlib#10547 matplotlib will change in version 2.2 (currently it is 2.2.rc1) the categorical code, so in my mind, this PR could only be merged, when pandas set the minimum required version of matplotlib to version 2.2. I do not know, what pandas convention is for requiring a minimum version of a library, especially in the case of matplotlib (The |
|
Our minimum matplotlib is pretty old. We usually work around bugs. @boeddeker does your test pass on matplotlib 2.2? I think the best thing to do is get pandas passing on matplotlib 2.2 (#19702) and skip this test on MPL older than 2.2. Could you also post images of the output on MPL 2.1 and MPL 2.2? |
|
I haven't installed matplotlib 2.2 because my cases work. When Version 2.2 is released, I will investigate how there categorical plot work and further work on this PR. |
|
@TomAugspurger should we fix / close this? |
|
We should get this passing and merged. @boeddeker can you track down why the CI tests failed? |
|
@TomAugspurger Currently I am busy, but in two weeks I have time. I will take care of this PR in two weeks. |
|
closing as stale, if you want to continue working, pls ping and we can re-open. you will need to merge master. |
git diff upstream/master -u -- "*.py" | flake8 --diffMatplotlib can handle string data, so
_get_xticksdoes not need to convert strings to int