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

Implementation of separate horizontal/vertical axes padding to the axis_grid toolkit #2466

Closed
wants to merge 8 commits into from

Conversation

matejak
Copy link
Contributor

@matejak matejak commented Sep 26, 2013

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.

@@ -20,6 +20,13 @@

#import numpy as np

def extend_axes_pad(value):
Copy link
Member

Choose a reason for hiding this comment

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

this probably should be _extend_axes_pad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, done.

@tacaswell
Copy link
Member

Sorry if I am stepping on any toes by commenting on this code.

This is similar in spirit to #2276 where I do something very similar (change a scalar parameter -> n-D) where I do it in the way I mention in the code comment (by adding extra scalar parameters).

@pelson
Copy link
Member

pelson commented Oct 11, 2013

@leejjoon - pinging you on this one. I'm in favour of this change, but I haven't read it completely, and I suspect you should actually press the merge button once your happy with it.

Cheers,

self._horiz_pad_size.fixed_size = axes_pad
self._vert_pad_size.fixed_size = axes_pad

self._init_axes_pad(axes_pad)
Copy link
Contributor

Choose a reason for hiding this comment

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

_init_axes_pad creates new Size.Fixed instances while set_axes_pad should not. Although I have not tested, I guess calling "set_axes_pad" will have no effect with this change.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I no longer understand my comment as my claim that they are identical is clearly wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is fixed now

@matejak
Copy link
Contributor Author

matejak commented Nov 26, 2013

I have used this feature successfully when writing a scientific paper, see Fig. 6

@tacaswell
Copy link
Member

@matejak This will need a re-base as it no longer merges cleanly.

@matejak
Copy link
Contributor Author

matejak commented Jan 8, 2014

I have not followed the advice on the web when making the fork and the pull request (I was not aware of those at that time), so I will drop this request and create a new one. Sorry about that.

@matejak matejak closed this Jan 8, 2014
@matejak matejak reopened this Jan 8, 2014
@tacaswell
Copy link
Member

@matejak What is the state of this?

@matejak matejak mentioned this pull request Jan 14, 2014
@matejak
Copy link
Contributor Author

matejak commented Jan 14, 2014

The state is good, there is another pull request #2731
The example works for me. The only gotcha can IMO be removal of unused imports in lib/mpl_toolkits/axes_grid1/axes_grid.py

@tacaswell tacaswell closed this Jan 15, 2014
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

4 participants