BUG: Plotting use FixedLocator #26185

nrebena · 2019-04-22T17:46:51Z

For string x values or multiindex, the axis locator is now set to a
FixedLocator for Line and Area plot. The ticklabel stay consistent
when scaling and zooming.

closes #7612
closes #15912
closes #22334

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

For string x values or multiindex, the axis locator is now set to a FixedLocator for Line and Area plot. The ticklabel stay consistent when scaling and zooming. This impact the following issues: GH7612, GH15912, GH22334

codecov · 2019-04-22T18:26:51Z

Codecov Report

Merging #26185 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #26185      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.02%     
==========================================
  Files         175      175              
  Lines       52371    52379       +8     
==========================================
+ Hits        48175    48176       +1     
- Misses       4196     4203       +7

Flag	Coverage Δ
#multiple	`90.53% <50%> (-0.01%)`	⬇️
#single	`40.69% <0%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`83.57% <50%> (-0.2%)`	⬇️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.71% <0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fecee8f...2e732b1. Read the comment docs.

codecov · 2019-04-22T18:26:57Z

Codecov Report

Merging #26185 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #26185      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.02%     
==========================================
  Files         175      175              
  Lines       52371    52379       +8     
==========================================
+ Hits        48175    48176       +1     
- Misses       4196     4203       +7

Flag	Coverage Δ
#multiple	`90.53% <50%> (-0.01%)`	⬇️
#single	`40.69% <0%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`83.57% <50%> (-0.2%)`	⬇️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.71% <0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fecee8f...2e732b1. Read the comment docs.

codecov · 2019-04-22T18:27:03Z

Codecov Report

Merging #26185 into master will decrease coverage by 0.02%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master   #26185      +/-   ##
==========================================
- Coverage   91.98%   91.96%   -0.03%     
==========================================
  Files         175      175              
  Lines       52371    52375       +4     
==========================================
- Hits        48175    48165      -10     
- Misses       4196     4210      +14

Flag	Coverage Δ
#multiple	`90.51% <55.55%> (-0.03%)`	⬇️
#single	`40.68% <0%> (-0.17%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`83.63% <55.55%> (-0.14%)`	⬇️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/indexing.py	`90.53% <0%> (-0.35%)`	⬇️
pandas/core/indexes/base.py	`96.72% <0%> (-0.27%)`	⬇️
pandas/core/computation/engines.py	`88.33% <0%> (-0.2%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/core/arrays/_ranges.py	`98.64% <0%> (-0.02%)`	⬇️
pandas/core/accessor.py	`98.79% <0%> (-0.02%)`	⬇️
pandas/core/internals/blocks.py	`94.07% <0%> (-0.02%)`	⬇️
pandas/io/formats/csvs.py	`98.19% <0%> (-0.02%)`	⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fecee8f...863ba3d. Read the comment docs.

WillAyd · 2019-04-23T02:33:11Z

Please add tests to cover the issues being closed

pep8speaks · 2019-04-23T12:37:27Z

Hello @nrebena! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-30 08:36:55 UTC

nrebena · 2019-04-23T12:49:03Z

I added tests for the issue. I used the example from #22334.

        df = pd.DataFrame({'sales': [3, 2, 3],
                           'visits': [20, 42, 28], 
                           'day': ['Monday', 'Tuesday', 'Wednesday']})
        ax = df.plot.area(x='day')
        ax.set_xlim(-1, 3)
        xticklabels = [t.get_text() for t in ax.get_xticklabels()]
        labels_position = dict(zip(xticklabels, ax.get_xticks()))
        # Testing if the label stayed at the right position
        assert labels_position['Monday'] == 0.0
        assert labels_position['Tuesday'] == 1.0
        assert labels_position['Wednesday'] == 2.0

This should guarantee that the xticklabel stay at the same position even after a change in the axis.
I am open for idea if there is a better way to test for the issues.

jreback

pls add a whatsnew note, can be a single one for all 3 issues if they are actually the same.
pls make sure that all of the issues have a test (unless they really are the same or your test covers all, just indicate this in a comment)

pandas/plotting/_core.py

pandas/tests/plotting/test_frame.py

Separate the test for pandas-dev#7612 and pandas-dev#22334, that are about string index, and test for pandas-dev#15912, about multiindex.

nrebena · 2019-04-24T14:18:55Z

I added the whatsnew entry, and splitted the test in two, one for string index and one for multiindex, as this was one of the difference between the issues, even if the solution is the same.

nrebena

Splitted the test in two, and moved the import.

jreback

minor doc comment; ping on green.

doc/source/whatsnew/v0.25.0.rst

jreback · 2019-04-30T10:23:08Z

does this fix #11465 ? (which is marked as a duplicate); can you add a test for this one as well.

nrebena · 2019-04-30T11:02:11Z

I already looked into it, and this doesnt fix #11465, and neither #2980. These are not duplicate and should maybe be reopen.
The way bar plot handle tick position is very different from how it is done in line plot, and it deal with ticks in its own peculiar way. I think I will look into them for my next PR hopefully 😃

jreback · 2019-04-30T12:59:58Z

thanks @nrebena

jreback · 2019-04-30T13:00:08Z

welcome patches for other issues!

ImportanceOfBeingErnest · 2019-04-30T16:07:55Z

pandas/plotting/_core.py

                ax.set_yticklabels(yticklabels)
+                ax.xaxis.set_major_locator(FixedLocator(yticks))
+                ax.xaxis.set_major_formatter(FixedFormatter(yticklabels))


Why is the x axis given the y-tick(label)s here? Seems like an error to me.
Meaning the "horizontal" direction is not sufficiently tested.

Also note that ax.set_xticklabels(xticklabels) and ax.xaxis.set_major_formatter(FixedFormatter(xticklabels)) are redundant. One of those lines should be enough.

This is definitly an error.
And this part is definitly not tested at all. None of the tests in plotting even hit this lines. This is because the self._need_to_set_index is only set to True for plot that have a self.orientation "vertical", so there is no way to go inside this loop.
What is the best course of action? Deleting this part that could never get hits?

If you have identified dead code you can submit a follow up PR to remove

BUG: Plotting use FixedLocator

2e732b1

For string x values or multiindex, the axis locator is now set to a FixedLocator for Line and Area plot. The ticklabel stay consistent when scaling and zooming. This impact the following issues: GH7612, GH15912, GH22334

gfyoung added Bug Visualization plotting labels Apr 22, 2019

TST: Add test for pandas-dev#7612 pandas-dev#22334 pandas-dev#15912

7cea50a

CLN: Fiex visual indent for PEP8

7e67f99

jreback requested changes Apr 24, 2019

View reviewed changes

pandas/plotting/_core.py Outdated Show resolved Hide resolved

pandas/tests/plotting/test_frame.py Outdated Show resolved Hide resolved

nicolas.rebena added 3 commits April 24, 2019 15:55

TST: Split test for pandas-dev#15912

3620080

Separate the test for pandas-dev#7612 and pandas-dev#22334, that are about string index, and test for pandas-dev#15912, about multiindex.

CLN: Fix import position in _post_plot_logic_common

f343631

DOC: Add whatsnew entry in Plotting

62874fb

nrebena commented Apr 27, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Apr 28, 2019

jreback requested changes Apr 28, 2019

View reviewed changes

doc/source/whatsnew/v0.25.0.rst Outdated Show resolved Hide resolved

DOC: Minor doc correction in whatsnew entry

863ba3d

jreback approved these changes Apr 30, 2019

View reviewed changes

This was referenced Apr 30, 2019

X axis weirdness with DataFrame.plot(kind='bar') #11465

Open

xtick label setting with improper scale #2980

Closed

jreback merged commit 3f02e63 into pandas-dev:master Apr 30, 2019

nrebena deleted the fix_7612 branch April 30, 2019 13:23

ImportanceOfBeingErnest reviewed Apr 30, 2019

View reviewed changes

nrebena mentioned this pull request May 1, 2019

CLN: Removing dead code in plotting and minor refactoring #26260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Plotting use FixedLocator #26185

BUG: Plotting use FixedLocator #26185

nrebena commented Apr 22, 2019 •

edited

Loading

codecov bot commented Apr 22, 2019

codecov bot commented Apr 22, 2019

codecov bot commented Apr 22, 2019 •

edited

Loading

WillAyd commented Apr 23, 2019

pep8speaks commented Apr 23, 2019 •

edited

Loading

nrebena commented Apr 23, 2019

jreback left a comment

nrebena commented Apr 24, 2019

nrebena left a comment •

edited

Loading

jreback left a comment

jreback commented Apr 30, 2019

nrebena commented Apr 30, 2019

jreback commented Apr 30, 2019

jreback commented Apr 30, 2019

ImportanceOfBeingErnest Apr 30, 2019

nrebena Apr 30, 2019

WillAyd May 1, 2019

BUG: Plotting use FixedLocator #26185

BUG: Plotting use FixedLocator #26185

Conversation

nrebena commented Apr 22, 2019 • edited Loading

codecov bot commented Apr 22, 2019

Codecov Report

codecov bot commented Apr 22, 2019

Codecov Report

codecov bot commented Apr 22, 2019 • edited Loading

Codecov Report

WillAyd commented Apr 23, 2019

pep8speaks commented Apr 23, 2019 • edited Loading

Comment last updated at 2019-04-30 08:36:55 UTC

nrebena commented Apr 23, 2019

jreback left a comment

Choose a reason for hiding this comment

nrebena commented Apr 24, 2019

nrebena left a comment • edited Loading

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented Apr 30, 2019

nrebena commented Apr 30, 2019

jreback commented Apr 30, 2019

jreback commented Apr 30, 2019

ImportanceOfBeingErnest Apr 30, 2019

Choose a reason for hiding this comment

nrebena Apr 30, 2019

Choose a reason for hiding this comment

WillAyd May 1, 2019

Choose a reason for hiding this comment

nrebena commented Apr 22, 2019 •

edited

Loading

codecov bot commented Apr 22, 2019 •

edited

Loading

pep8speaks commented Apr 23, 2019 •

edited

Loading

nrebena left a comment •

edited

Loading