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

Fix shifted ylabels (Issue #1571) #1589

Merged
merged 1 commit into from
Jan 16, 2013

Conversation

dopplershift
Copy link
Contributor

Account for rotation mode when setting axis label position. This is used, for instance, when setting labels on colorbars.

@dopplershift
Copy link
Contributor Author

dopplershift commented Dec 12, 2012

I'm not necessarily sure this is the right way to fix this, as the logic for how text alignment interacts with with how rotation is performed is getting a little hairy--or at least it's bleeding through from the text object to the axis code with this fix. I'm personally not convinced the change to "anchored" rotation was fully baked given that core functionality (like colorbar labelling) was completely borked by the change.

IMO, it might make more sense to make handle any change to alignment internally in the Text object and let vertical/horizontal alignment work the same (and as expected) in all cases regardless of rotation_mode. However, I'm not as familiar with that code or any issues behind rotation_mode.

At any rate, I just want unbroken plots, so if this is the agreed to way forward, let's just get this in....

@ghost ghost assigned pwuertz Dec 12, 2012
@pmarshwx
Copy link

As someone who uses MPL for all of my research plots, having the y-labels broken in master is pretty disheartening...and frustrating. @dopplershift, your fix works for me, under the tests I've put it through, so unless someone has a major objection to how you implemented the fix, I think this is something that should get put in fairly quickly to get master back to working properly.

With this said, I do see that the Travis build fails on this commit, so maybe there is a better way?

@dopplershift
Copy link
Contributor Author

The Travis failure looks unrelated--the failure is in a test about invisible lines.

@ivanov
Copy link
Member

ivanov commented Dec 15, 2012

Looks like this came from #1081, and the default horizontal and vertical alignments were changed, along with the introduction of the new rotation mode. Does changing the default ha and va help fix the issue here?

Let's not rush on this fix until we know it's the right one, since there was already one other fix to #1081.

@dopplershift
Copy link
Contributor Author

The problem is that choosing the correct alignment depends on whether rotation is done before or after the alignment. This PR puts logic to handle this into the 'Axis' class. I'm not sure if other users of text objects need this or not.

@dopplershift
Copy link
Contributor Author

I'd certainly like a comment from @pwuertz, since he introduced the new functionality (and broke my colorbar labels 😏 )

@pwuertz
Copy link
Contributor

pwuertz commented Dec 21, 2012

Sorry sorry sorry! The past few weeks were pretty busy and eventful, else I'd have commented on this a lot sooner!

Ok, so my intent was definitely not to mess up the ylabels, and an extended test coverage like the one @jenshnielsen wants to merge in #1422 would probably discover such errors sooner.

So about the rotation issue. I think your patch is perfectly reasonable and doesn't contain unnecessary Text object interactions. I think it could even be simplified by setting rotation mode to anchor, horizontal alignment to center, and vertical alignment to either top or bottom (since this function seems to re-do the layout anyways?).

The motivation for this anchor-rotation is to enable proper treatment of texts in backends, or to enable new ways of alignment in matplotlib itself. First of all, it's easier to implement, since in SVG and TiKZ/PGF it is also the preferred way of rotating texts. Second, aligning a rotated text by its bounding box prevents you from doing vertical alignment by baseline.

As far as I can see, for Y-labels there is no drawback in choosing anchored rotation mode... well, except from breaking functions I don't know of that rely on the traditional rotation mode. But you could say that this is a bug in these functions since they should have checked for the rotation_mode attribute supported by matplotlib ;) .

@dopplershift
Copy link
Contributor Author

No worries, I completely understand being busy. (I also know the best way to get me to respond is to keep making noise.)

I think I'm ok with just using the anchored rotation to set label position, provided we also set it on the label object within set_label_position().

I can implement this, but before I do, does anyone else want to weigh in here?

@pmarshwx
Copy link

pmarshwx commented Jan 1, 2013

Merge it!

Force 'anchor' rotation mode when setting the label position,
and use the appropriate alignment for the Y-label position.
dopplershift added a commit that referenced this pull request Jan 16, 2013
@dopplershift dopplershift merged commit 8ea6241 into matplotlib:master Jan 16, 2013
@dopplershift
Copy link
Contributor Author

Ok, I've gone ahead and just forced the rotation mode to 'anchor' (and horizontal alignment to 'center') and set the vertical alignment as appropriate. This seems fine since the axis creates its labels from the start; setting the label position is a property of the axis, not the label, so it should be free to set any/all properties.

Since this is so close to the last version, I'm going ahead and merging since I've now seen this on the ML.

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