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

2d padding #2731

Merged
merged 7 commits into from Feb 27, 2014
Merged

2d padding #2731

merged 7 commits into from Feb 27, 2014

Conversation

matejak
Copy link
Contributor

@matejak matejak commented Jan 14, 2014

Used to be here: #2466 (comment)

This feature is useful in cases such as showing the same section of an image with different colorbar ranges.
Since colorbars have to be labelled, it is not possible to get things right when using the same horizontal and vertical spacing.
I attach an image (you can obtain it by running the demo script or by generating the documentation, the new functionality is the image to the right).
padding

I think that I haven't broken anything since the documentation generation did not complain. However, I don't know how to write tests for this feature.

@tacaswell
Copy link
Member

The failures look unrelated for these changes.

I closed the other PR.

self.axes_column = [[] for i in range(self._ncols)]
self.axes_row = [[] for i in range(self._nrows)]
self.axes_column = [[] for _ in range(self._ncols)]
self.axes_row = [[] for _ in range(self._nrows)]
Copy link
Member

Choose a reason for hiding this comment

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

why the variable change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because i is not used in the expression, I have replaced it with _, which is considered as a "throw-away" variable identifier (see for instance http://stackoverflow.com/questions/1739514/as-variable-name-in-python)

@tacaswell
Copy link
Member

I have a few concerns about code style, mostly the inline if statements and the change of inner variables to _.

Functionality wise, looks good to me.

This module (and basically all of mpl_toolkits) currently has no test coverage, we don't necessarily want to start in this PR, so don't worry about the tests at this point.

@matejak
Copy link
Contributor Author

matejak commented Jan 15, 2014

I have fixed the controversial things except renames to _ (at least for the time being) for reasons mentioned above.

@matejak
Copy link
Contributor Author

matejak commented Jan 22, 2014

Hi there, do I have to do something now to get this moving again (such as writing about it to the mailing list)?

@tacaswell
Copy link
Member

@matejak Hopefully I will get this merged very soon (travis is not loading for me tonight...).

I still do not like the i -> _ change, I think it makes it less readable.

@@ -335,8 +345,8 @@ def _update_locators(self):
v = []

v_ax_pos = []
v_cb_pos = []
for ax in self._row_refax[::-1]:
v_cbI#_pos = []
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a '#' in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For your reference, it was Vim typo (pressed I followed by # thinking that I am in normal mode)

@matejak
Copy link
Contributor Author

matejak commented Feb 26, 2014

I dare to disagree about the underscore (ie _), from my POV, it is a clear message saying "this variable is dummy, we claim that it is not used anywhere" and syntax checkers such as pylint are aware of this.
Nevertheless, if you make a clear statement that it should be reverted to the original name, I will do so immediately.

@tacaswell
Copy link
Member

I don't feel strongly enough to demand you change the variables back, but that vim typo should probably be fixed.

Can you also add entries to CHANGELOG, api_changes.rst, and whats_new.rst?

@matejak
Copy link
Contributor Author

matejak commented Feb 26, 2014

I have rebased to the current master (hopefully, I am not so closely familiar with Git) and also committed descriptions you have kindly suggested.
That's for it today, please let me know whether something still has to be done.

tacaswell added a commit that referenced this pull request Feb 27, 2014
@tacaswell tacaswell merged commit 4d53538 into matplotlib:master Feb 27, 2014
@matejak matejak deleted the 2d-padding branch February 27, 2014 11:26
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.

None yet

2 participants