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

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

Closed
wants to merge 7 commits into from

Conversation

efiring
Copy link
Member

@efiring efiring commented Jun 12, 2017

  • 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

- 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
Copy link
Member

attn @QuLogic and @astrofrog

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jun 12, 2017
@efiring
Copy link
Member Author

efiring commented Jun 13, 2017

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
Copy link
Member

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

@QuLogic
Copy link
Member

QuLogic commented Jun 13, 2017

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

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

Errant comment character.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@@ -1093,6 +1081,13 @@ def cla(self):
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
Copy link
Member

Choose a reason for hiding this comment

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

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...

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)
Copy link
Member

Choose a reason for hiding this comment

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

Match opening parenthesis?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

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?
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it should be in __init__.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, comment has been deleted.

axis.set_minor_formatter(NullFormatter())
# update the minor locator for x and y axis based on rcParams
if (rcParams['xtick.minor.visible']):
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary parentheses.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@efiring
Copy link
Member Author

efiring commented Jun 15, 2017

@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.

This is required for consistency with the present grid() documentation
and behavior, but the list of allowed properties should be trimmed.
@efiring
Copy link
Member Author

efiring commented Jun 27, 2017

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

@tacaswell
Copy link
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

so

on this branch.

This is to allow twinx and twiny to work as expected.
@efiring
Copy link
Member Author

efiring commented Jul 3, 2017

@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 tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@jklymak
Copy link
Member

jklymak commented Dec 12, 2017

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
Copy link
Member Author

efiring commented Jun 11, 2018

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 efiring closed this Jun 11, 2018
@dstansby dstansby removed this from the v3.0 milestone Jun 11, 2018
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

5 participants