Skip to content

moved cla shared axes code to a dedicated function #8710

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

Closed
wants to merge 2 commits into from
Closed

moved cla shared axes code to a dedicated function #8710

wants to merge 2 commits into from

Conversation

jrmlhermitte
Copy link
Contributor

@jrmlhermitte jrmlhermitte commented Jun 3, 2017

PR Summary

Response to the end of #8455 with discussion with @WeatherGod

The goal of this PR is to clean up the code in Axes.cla(), which has grown to be large. We probably want something clean over something quick here. This is meant to be a very simple aesthetic change, nothing more (no logic changes).

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

PR Details

This is also just a suggestion, and there likely may be a better way. I am opening it so it does not get lost. I am open to suggestions, and if you have something nicer, feel free to create a new PR (but please reference this one so we don't get lost).

However, I believe it does bring some points of discussion that I think may be necessary to address, such as:

  • I think I may be breaking convention (from what I see in code) by having the function take a string and use it to call class methods, but so far it seems to be a cleaner and still readable to me.
  • If cla is overridden, then _cla_shared needs to be also removed from the class (i.e. for polar axes). Right now this would have to be manually done. I am wondering if there exist decorators implemented that take care of these? For example:
def MyNewClass(OldClass):
    @cla.override
    def cla(self):
        #....

where @cla.override would contain the necessary bookeeping to remove _cla_shared when cla is overridden. I will try to search more when I have time but if someone has insight I'm interested to know (and thanks for saving me time :-) ). @QuLogic might have some idea and some input with his experience with polaraxes.

  • Will need to wait for Travis. My local tests on test/test_axes.py contain a long list of failures (which seem to be the same with and without this change).

thanks!

@anntzer
Copy link
Contributor

anntzer commented Jun 3, 2017

I would something like

for shared, axis in [(self._sharex, self.xaxis), (self._sharey, self._sharey)]:
    ...

(where get/set_x/ylim can be written as axis.get/set_view_interval() (both are ultimately accessing ax.viewLim.intervalx/y).

@efiring
Copy link
Member

efiring commented Jun 3, 2017

I think there is a much simpler way to eliminate most of the duplication that this PR is targeting. The problem is in the calls to the Axis._set_scale() method. That method is doing only one thing that makes sense here, and everything else that it is doing is having to be undone, resulting in the horrible bulk of code to save and restore the formatters and locators. I think that if the two calls to that method were replaced by calls to the one line we want, e.g. for xaxis:

#      self.xaxis._set_scale(self._sharex.xaxis.get_scale())
        self.xaxis._scale = mscale.scale_factory(self._sharex.xaxis.get_scale(), self.xaxis)

then the saving and restoring could be deleted, saving 16 lines of code.
I haven't tested this, so I might not have it exactly right, or I might be missing something.

efiring added a commit to efiring/matplotlib that referenced this pull request Jun 4, 2017
jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Jun 13, 2017
@anntzer
Copy link
Contributor

anntzer commented Jun 23, 2017

Overridden by #8720.

@anntzer anntzer closed this Jun 23, 2017
@jrmlhermitte jrmlhermitte deleted the cla-shared branch June 24, 2017 01:25
@jrmlhermitte
Copy link
Contributor Author

thanks for letting me know. I apologize, I have been extremely busy with other things. I'm glad this got sorted out :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants