Skip to content
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

DOC: Correct subplot() doc #7760

Merged
merged 3 commits into from Jan 10, 2017
Merged

DOC: Correct subplot() doc #7760

merged 3 commits into from Jan 10, 2017

Conversation

petehuang
Copy link
Contributor

Fixes #7393

Creating a new subplot with a position which is entirely inside a
pre-existing axes will trigger the larger axes to be deleted::
Creating a new subplot with axes that overlap pre-existing axes will
trigger the pre-existing axes to be deleted::
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like "Creating a subplot will delete any pre-existing subplot that overlaps with it." seems clearer (possibly add "(if any)" at the end).

@petehuang
Copy link
Contributor Author

Good suggestion @anntzer

@codecov-io
Copy link

codecov-io commented Jan 7, 2017

Current coverage is 62.11% (diff: 100%)

Merging #7760 into master will decrease coverage by <.01%

@@             master      #7760   diff @@
==========================================
  Files           174        174          
  Lines         56028      56028          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34805      34803     -2   
- Misses        21223      21225     +2   
  Partials          0          0          

Powered by Codecov. Last update 1fa4dd7...4abaa24

@anntzer anntzer changed the title DOC: Correct subplot() doc [MRG+1] DOC: Correct subplot() doc Jan 7, 2017
@anntzer
Copy link
Contributor

anntzer commented Jan 7, 2017

lgtm

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 7, 2017
@@ -976,8 +976,8 @@ def subplot(*args, **kwargs):

.. note::

Creating a new subplot with a position which is entirely inside a
pre-existing axes will trigger the larger axes to be deleted::
Creating a subplot will delete any pre-existing subplot that overlaps
Copy link
Member

Choose a reason for hiding this comment

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

Overlaps or fully overlaps?

@NelleV
Copy link
Member

NelleV commented Jan 8, 2017

Thanks @petehuang

@NelleV NelleV changed the title [MRG+1] DOC: Correct subplot() doc [MRG+2] DOC: Correct subplot() doc Jan 8, 2017
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

Please hold off. I think that both the code and the documentation are incomprehensible.

@efiring
Copy link
Member

efiring commented Jan 8, 2017

I challenge anyone reading the proposed modification of the docstring, without poring over the code, to explain what "fully overlaps" means. Before reading the code, I was imagining that it might mean "encloses", as the original form of the docstring implied. Based on the Bbox method code, however, it means that the interiors intersect--that is, the intersection of the two rectangles encloses a finite area. Hence the subplot code is deleting any pre-existing subplots that have a finite intersection with the new subplot. Coinciding edges are not enough, because then the intersection encloses no area.

@petehuang
Copy link
Contributor Author

Ok let me paraphrase to make sure I understand correctly: you're saying that it's not determined by axes lying over one another, but it's that the rectangles that the axes create intersect in some way?

I just want to make sure we get this explanation and the docstring in plain english

@efiring
Copy link
Member

efiring commented Jan 8, 2017

The practical problem is coming from the adverb "fully". The "overlaps" part is fine. The subtle distinction is that a shared boundary does not count as an "overlap". You could change "that fully overlaps with it" to "that has a finite area of overlap with it". Or "that overlaps with it beyond sharing a boundary".

@NelleV
Copy link
Member

NelleV commented Jan 9, 2017

I knew what the function did (and I find this behavior extremely strange…).
I would actually raise a warning to the user when that happens, as this behavior is quite unexpected.

@efiring efiring changed the title [MRG+2] DOC: Correct subplot() doc DOC: Correct subplot() doc Jan 10, 2017
@efiring efiring merged commit d75972e into matplotlib:master Jan 10, 2017
@efiring
Copy link
Member

efiring commented Jan 10, 2017

Thank you!

@QuLogic
Copy link
Member

QuLogic commented Jan 30, 2017

Backported to v2.0.x as 98c374d.

QuLogic pushed a commit that referenced this pull request Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants