Skip to content

2d padding - #2731

Merged
tacaswell merged 7 commits into
matplotlib:masterfrom
matejak:2d-padding
Feb 27, 2014
Merged

tacaswell merged 7 commits into
matplotlib:masterfrom
matejak:2d-padding

Conversation

@matejak

@matejak matejak commented Jan 14, 2014

Copy link
Copy Markdown
Contributor

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

The failures look unrelated for these changes.

I closed the other PR.

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.

why the variable change?

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

matejak commented Jan 15, 2014

Copy link
Copy Markdown
Contributor Author

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

@matejak

matejak commented Jan 22, 2014

Copy link
Copy Markdown
Contributor Author

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

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.

Why is there a '#' in here?

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

matejak commented Feb 26, 2014

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

matejak commented Feb 26, 2014

Copy link
Copy Markdown
Contributor Author

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.

2 participants