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

Refactor BasePlotter's wrapping of Renderer #552

Merged
merged 9 commits into from
Jan 22, 2020
Merged

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented Jan 20, 2020

Close #470 and follow up #536 (comment)

This uses functools.wraps to properly wrap the methods on BasePlotter that call to the active Renderer, minimizing duplicated docstrings and argument listing. Note that properties still aren't handled via wrapping but are all explicitly defined in BasePlotter.

I also went ahead and deprecated the loc argument that was in a few places across the BasePlotter API as it was mostly unused. This deprecation was done in favor of having the user activate a given plotter by the subplot method then calling the method of choice. See #536 (comment). I think that using one active renderer managed by subplot() is more in line with our mission to follow Matplotlib conventions and simpler (having loc argument just makes things messy and hard to manage). Also, this is in line with the Zen of Python:

There should be one-- and preferably only one --obvious way to do it.

I also reorganized the Plotter and Renderer class by keeping all of the wrapped methods from Renderer and properties in their respective sections of the class definition.

@banesullivan banesullivan added deprecation Deprecation involved. PRs that deprecate things. documentation Anything related to the documentation/website enhancement Changes that enhance the library priority-high This is a fairly serious issue/feature request that needs to be addressed. proposed-change Something with regards to the API or internal structure is changing. labels Jan 20, 2020
@banesullivan banesullivan self-assigned this Jan 20, 2020
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Excellent, this is just want we need to reduce the complexity of BasePlotter. Thanks for wrapping it, I see no problems with the code, and removing the loc kwarg is a good move as it appears to be unused.

@banesullivan
Copy link
Member Author

@GuillaumeFavelier, I did a quick look through mne-python and it looks like you all never used the loc argument but just the subplot() method so all should be good. Either way, it would be great to have your eyes on this too

Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier left a comment

Choose a reason for hiding this comment

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

LGTM. Great work refactoring all this @banesullivan. Indeed we choose to use subplot() instead of loc in mne-python 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Deprecation involved. PRs that deprecate things. documentation Anything related to the documentation/website enhancement Changes that enhance the library priority-high This is a fairly serious issue/feature request that needs to be addressed. proposed-change Something with regards to the API or internal structure is changing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants