FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Simplify axes and axis clearing, sharing, and aspect ratio control by efiring · Pull Request #8752 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Simplify axes and axis clearing, sharing, and aspect ratio control - #8752

Closed
efiring wants to merge 7 commits into
matplotlib:masterfrom
efiring:simplify_cla_share_part2
ConversationCommits (7)ChecksFiles changed
Closed

Simplify axes and axis clearing, sharing, and aspect ratio control#8752
efiring wants to merge 7 commits into
matplotlib:masterfrom
efiring:simplify_cla_share_part2

Conversation

efiring commented Jun 12, 2017

Copy link
Copy Markdown
Member
  • Most of the unnecessary calls to Axis.cla() have been removed.
  • Axis sharing works across figures.
  • The "box-forced" adjustable is no longer needed.
  • Sharing both axes requires the use of "box", not "datalim".
  • A new "share" kwarg triggers synchronized setting of aspect
    ratio and adjustable in Axes within shared axis groups.
  • Added a test for axis sharing with aspect ratio setting.
  • Fixed and updated skew_rects test.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/whats_new.rst if major new feature
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Simplify axes and axis clearing, sharing, and aspect ratio control
74f6526 
- Most of the unnecessary calls to Axis.cla() have been removed.
- Axis sharing works across figures.
- The "box-forced" adjustable is no longer needed.
- Sharing both axes requires the use of "box", not "datalim".
- A new "share" kwarg triggers synchronized setting of aspect
  ratio and adjustable in Axes within shared axis groups.
- Added a test for axis sharing with aspect ratio setting.
- Fixed and updated skew_rects test.

tacaswell commented Jun 12, 2017

Copy link
Copy Markdown
Member

attn @QuLogic and @astrofrog

tacaswell added this to the 2.1 (next point release) milestone Jun 12, 2017

efiring commented Jun 13, 2017

Copy link
Copy Markdown
Member Author

Odd failure ("image file not readable") on the doc build:

Warning, treated as error:

/home/travis/build/matplotlib/matplotlib/doc/tutorials/01_introductory/sample_plots.rst::image file not readable: tutorials/01_introductory/../../gallery/lines_bars_and_markers/images/sphx_glr_fill_001.png

dstansby commented Jun 13, 2017

Copy link
Copy Markdown
Member

Looks like some PEP8 errors too https://travis-ci.org/matplotlib/matplotlib/jobs/242143280#L2558

QuLogic commented Jun 13, 2017

Copy link
Copy Markdown
Member

Some of these changes are similar to #8626, though perhaps with slightly different triggers.

QuLogic reviewed Jun 13, 2017
View reviewed changes

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't really know much about the sharing aspects; I looked mostly at the axis bits.

Comment thread lib/matplotlib/axes/_base.py Outdated

if clear_axis:
self.xaxis.cla(shared_x)
self.yaxis.cla(shared_y) #

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Errant comment character.

efiring Jun 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

removed

Comment thread lib/matplotlib/axes/_base.py
self.yaxis.set_visible(yaxis_visible)
self.patch.set_visible(patch_visible)

# It is not clear to me (EF) why this reset is needed here, but

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Axes, Axis, Spine, and Tick are all inexplicably intertwined. I've been trying to disconnect them (e.g., #8678), but there's always one last bug every time...

Comment thread lib/matplotlib/axis.py Outdated
self._grid_linewidth = (rcParams['grid.linewidth']
if grid_linewidth is None else grid_linewidth)
self._grid_alpha = (rcParams['grid.alpha']
if grid_alpha is None else grid_alpha)

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Match opening parenthesis?

efiring Jun 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

Comment thread lib/matplotlib/axis.py Outdated
self.set_label_text('')
self._set_artist_props(self.label)
self.label.set_text('') # self.set_label_text would change isDefault_
self._set_artist_props(self.label) # sets figure; needed here?

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems like it should be in __init__.

efiring Jun 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually it is already in _get_label, so I don't think we need it in cla or init.

Comment thread lib/matplotlib/axis.py Outdated

if reset:
self.reset_ticks()
# Is this "else" correct? Shouldn't kwargs be applied after a reset?

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

reset_ticks deletes all ticks; they'll get recreated with the updated parameters from the dictionary.

efiring Jun 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, comment has been deleted.

Comment thread lib/matplotlib/scale.py Outdated
axis.set_minor_locator(NullLocator())
axis.set_minor_formatter(NullFormatter())
# update the minor locator for x and y axis based on rcParams
if (rcParams['xtick.minor.visible']):

QuLogic Jun 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unnecessary parentheses.

efiring Jun 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

removed

efiring commented Jun 15, 2017

Copy link
Copy Markdown
Member Author

@QuLogic, thank you for the review. I have addressed the minor points. I'm sorry for the overlap with your work; I haven't been keeping up with existing PRs.

efiring added 3 commits June 15, 2017 17:50
Single extra space removed to placate PEP 8.
bff7e12
Handle all Line2D properties for gridlines.
f493ac3 
This is required for consistency with the present grid() documentation
and behavior, but the list of allowed properties should be trimmed.
PEP 8 again
d575d60

efiring commented Jun 27, 2017

Copy link
Copy Markdown
Member Author

This would close #2790; the test case described at the top of that issue works correctly.

tacaswell commented Jul 2, 2017

Copy link
Copy Markdown
Member

I do not think forcing shared axes to 'box' is right

import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax2 = ax.twinx()
ax.set_aspect(5)

gives

on this branch.

API: re-allow 'datalim' as a valid adjustable with shared axes
7dba686 
This is to allow twinx and twiny to work as expected.

efiring commented Jul 3, 2017

Copy link
Copy Markdown
Member Author

@tacaswell, yes, the twinx situation has to be handled differently. This arose recently in another issue as well. Twinx needs to ensure that the two axes have the same position, even if the position of one or the other is changed by any means.

tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
efiring mentioned this pull request Sep 1, 2017
Closed
4 of 6 tasks
This was referenced Nov 8, 2017
Merged
Closed
efiring mentioned this pull request Dec 4, 2017
Closed
6 tasks done

jklymak commented Dec 12, 2017

Copy link
Copy Markdown
Member

Pinging on the status of this PR or if there is any way I can help. I've assiduously avoided the aspect code in #9082 (layout manager) but it'll come up at some point. Knowing what the API is and what the current code is meant to do would be great...

efiring mentioned this pull request Dec 17, 2017
Merged
efiring mentioned this pull request Jan 8, 2018
Merged
5 of 6 tasks
jklymak modified the milestones: needs sorting, v3.0 May 17, 2018

efiring commented Jun 11, 2018

Copy link
Copy Markdown
Member Author

Parts of this have come in via other PRs. The part that hasn't--the reduction of unnecessary clearing--will require a fresh look. Closing.

efiring closed this Jun 11, 2018
dstansby removed this from the v3.0 milestone Jun 11, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic left review comments

Assignees

No one assigned

Labels

status: needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL