Skip to content

[MRG+1] plot_date() after axhline() doesn't rescale axes #8205

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

[MRG+1] plot_date() after axhline() doesn't rescale axes #8205

wants to merge 12 commits into from

Conversation

Yasaman-Mah
Copy link

@Yasaman-Mah Yasaman-Mah commented Mar 6, 2017 •
edited by NelleV
Loading

The only reason that plotting axhline first changes behaviour of plotting datetime is that it sets ignore_existing_data_limits to false and then when plotting datetime data, default datetime data limits are taken into consideration.
Unfortunately this fix would affect y data limits too. I think a better solution would be to have two variables ignore_existing_x_limits and ignore_existing_y_limits, but I am not sure if adding more detail and making it slightly more complicated just because of axhline makes sense.

ref #7742

phobson
phobson reviewed Mar 6, 2017
View reviewed changes
lib/matplotlib/tests/test_axes.py Outdated
@@ -4972,3 +4972,21 @@ def test_bar_single_height():
ax.bar(range(4), 1)
# Check that a horizontal chart with one width works
ax.bar(0, 1, bottom=range(4), width=1, orientation='horizontal')

def test_datetime_axhline_same_axes():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pep8 checker is probably going to want 2 blank lines between functions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you'll also need to trim trailing whitespace:
https://travis-ci.org/matplotlib/matplotlib/jobs/208246492#L1683

@NelleV NelleV changed the title issue #7742 plot_date() after axhline() doesn't rescale axes Mar 6, 2017
@NelleV
Copy link
Member

NelleV commented Mar 6, 2017

Next time, please add a meaningful title to the pull requests. The reference to the ticket should be in in the description of the PR (else it is non clickable).

@phobson phobson mentioned this pull request Mar 7, 2017
Closed
ibnIrshad referenced this pull request in sughandj/matplotlib Mar 7, 2017
@sughandj
Fixes matplotlib#7742 - axhline rescale issue
7cf097d 
Modified axhline() function to save state of ignoring limits and restore after plotting the line
@ibnIrshad ibnIrshad mentioned this pull request Mar 7, 2017
Merged
@Shang-Jia
Copy link

This solution will always reset the existing_data_limits so on the following execution
plot() -> sets data limits
axhline() -> allows data limits to be set again
plot() -> sets data limits again, even if data limits are smaller than what was previously plotted
see testcase and proposed solution in #8210

@Yasaman-Mah
Copy link
Author

@Shang-Jia This is a preliminary solution, I submitted this pull request to get some feedback from matplotlib team before going on with another solution.

@NelleV
Copy link
Member

NelleV commented Mar 11, 2017

This looks good to me. Thanks for the patch and sorry it took so long to get feedback!

@NelleV NelleV changed the title plot_date() after axhline() doesn't rescale axes [MRG+1] plot_date() after axhline() doesn't rescale axes Mar 11, 2017
tacaswell
tacaswell reviewed Mar 12, 2017
View reviewed changes
lib/matplotlib/axes/_axes.py
@@ -725,6 +725,7 @@ def axhline(self, y=0, xmin=0, xmax=1, **kwargs):
trans = self.get_yaxis_transform(which='grid')
l = mlines.Line2D([xmin, xmax], [y, y], transform=trans, **kwargs)
self.add_line(l)
self.ignore_existing_data_limits = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a symmetric change to axvline ?

Copy link
Author

@Yasaman-Mah Yasaman-Mah Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a similar change to axvline. Please see my comment below before approving the changes.

@Yasaman-Mah
Copy link
Author

This solution has a side effect, it will result in inconsistent behaviour when setting y axis limits. For example:

`import matplotlib.pyplot as plt

fig, axs = plt.subplots(2, 1)

axs[0].axvline(1)
axs[0].plot([2, 3], [2, 3])

axs[1].plot([2, 3], [2, 3])
axs[1].axvline(1)

plt.show()`

will result in following plots:
screenshot from 2017-03-15 20 36 29
which are not wrong but inconsistent.
However, I believe currently there is no way to avoid this unless we add one more field to axes object or make more substantial changes to how limits are set.

@ibnIrshad
Copy link

ibnIrshad commented Mar 18, 2017 •
edited
Loading

@Yasaman-Mah It's good you pointed that out. However I still think that this PR is a useful contribution and should be merged.

This PR fixes #7742 (which produces clearly wrong output), and although it does introduce a side effect, the side effect is not "wrong output", but only slightly inconsistent for a certain specific order of input. And as you pointed out, it would require MUCH more work to fix this inconsistency, because introducing two symmetric flags "ignore_y_limits" and "ignore_x_limits" is a significant architectural change.

Hence I would ask @NelleV @tacaswell to review this PR again. Can we go ahead with this fix?

@tacaswell
Copy link
Member

What are dates (which are just input with units) behaving differently than plain numbers?

I am still not sure I understand what the self.ignore_existing_data_limits flag does and when it gets set/unset.

The code and the figure above seem out of sync with each other, but I would not call that a 'side effect'. IT looks like the bug from #7742 (order dependent limits) has just been moved to different (and probably more common) case.

tacaswell
tacaswell requested changes Mar 18, 2017
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure I understand what is going on here.

Anyone who can may clear this review.

@dstansby dstansby added this to the 2.1.1 (next bug fix release) milestone Aug 4, 2017
@tacaswell tacaswell removed this from the 2.1.1 (next bug fix release) milestone Oct 9, 2017
@tacaswell tacaswell added this to the 2.2 (next feature release) milestone Oct 9, 2017
@jklymak
Copy link
Member

jklymak commented Mar 24, 2018

Can this PR be resurrected or closed?

@jklymak jklymak modified the milestones: needs sorting, v3.0 Mar 24, 2018
@tacaswell tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018
@jklymak
Copy link
Member

jklymak commented Feb 7, 2019

I'm closing for now - but feel free to re-open if someone wants to champion this PR. Thanks for the contribution.

@jklymak jklymak closed this Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phobson phobson phobson left review comments

@tacaswell tacaswell tacaswell requested changes

@NelleV NelleV NelleV approved these changes

Assignees
No one assigned
Labels
topic: date handling
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Yasaman-Mah @NelleV @Shang-Jia @ibnIrshad @tacaswell @jklymak @phobson @dstansby @anntzer