Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes issue #1960. Account for right/top spine data offset on transform ... #1964

Merged
merged 3 commits into from May 11, 2013

Conversation

Projects
None yet
3 participants
Contributor

cimarronm commented Apr 30, 2013

...when doing spine.set_position(). Also includes a testcase for the data locations.

@cimarronm cimarronm Fixes issue #1960. Account for right/top spine data offset on transfo…
…rm when doing spine.set_position(). Also includes a testcase for the data locations.
247689d

@pelson pelson and 2 others commented on an outdated diff May 1, 2013

lib/matplotlib/spines.py
@@ -323,6 +323,8 @@ def _calc_offset_transform(self):
self._spine_transform = ('identity',
mtransforms.IdentityTransform())
elif position_type == 'data':
+ if self.spine_type in ('right', 'top'):
+ amount -= 1 # translate left by one to account for right/top data offset of one
@pelson

pelson May 1, 2013

Member

Sorry, the comment doesn't enlighten me. Do you know the root cause of this workaround? Is this one pixel? Are you certain that this isn't a snapping issue?

Apologies for the (possibly silly) questions - I would like to understand the underlying need for this change.

@mdboom

mdboom May 1, 2013

Owner

It's not one pixel -- it's in axes units which are (0, 1) across the entire axis. Maybe a better comment would be:

The right and top spines have a default position of 1 in axes coordinates.  When specifying the position in data coordinates, we need to calculate the position relative to 0.
@cimarronm

cimarronm May 1, 2013

Contributor

That is correct. I updated the code with Michael's suggested comment. Thanks @mdboom

@pelson pelson commented on the diff May 1, 2013

lib/matplotlib/spines.py
@@ -323,6 +323,11 @@ def _calc_offset_transform(self):
self._spine_transform = ('identity',
mtransforms.IdentityTransform())
elif position_type == 'data':
+ if self.spine_type in ('right', 'top'):
+ # The right and top spines have a default position of 1 in
+ # axes coordinates. When specifying the position in data
+ # coordinates, we need to calculate the position relative to 0.
+ amount -= 1
@pelson

pelson May 1, 2013

Member

Thanks @cimarronm - this is much clearer now.

Member

pelson commented May 1, 2013

Just needs an doc/api/api_changes.rst entry since this will change some functionality (for the better - by fixing a bug) and I think it is good to go.

Nice work @cimarronm.

👍

Contributor

cimarronm commented May 1, 2013

How about:

* Fixed a bug in setting the position for the right/top spine with data 
  position type. Previously, it would draw the right or top spine at
  +1 data offset.

Let me know if that is good or want to improve the wording for doc/api/api_changes.rst

Member

pelson commented May 2, 2013

Let me know if that is good or want to improve the wording for

That sounds good to me. The api_changes.rst document is there for those who are finding problems when upgrading versions, so something short and to the point is exactly what is needed. Thanks @cimarronm

Contributor

cimarronm commented May 10, 2013

Is this good to merge?

@mdboom mdboom added a commit that referenced this pull request May 11, 2013

@mdboom mdboom Merge pull request #1964 from cimarronm/data_spine_right_top_location…
…_fix

Fixes issue #1960. Account for right/top spine data offset on transform ...
af07100

@mdboom mdboom merged commit af07100 into matplotlib:master May 11, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment