X Tutup
Skip to content

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

Merged
mdboom merged 3 commits intomatplotlib:masterfrom
cimarronm:data_spine_right_top_location_fix
May 11, 2013
Merged

Fixes issue #1960. Account for right/top spine data offset on transform ...#1964
mdboom merged 3 commits intomatplotlib:masterfrom
cimarronm:data_spine_right_top_location_fix

Conversation

@cimarronm
Copy link
Copy Markdown
Contributor

@cimarronm cimarronm commented Apr 30, 2013

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

@cimarronm
Fixes issue matplotlib#1960. Account for right/top spine data offset …
247689d 
…on transform when doing spine.set_position(). Also includes a testcase for the data locations.
pelson
pelson reviewed May 1, 2013
View reviewed changes
lib/matplotlib/spines.py Outdated
Copy link
Copy Markdown
Member

@pelson pelson May 1, 2013

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mdboom mdboom May 1, 2013

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@cimarronm cimarronm May 1, 2013

Choose a reason for hiding this comment

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

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

pelson
pelson reviewed May 1, 2013
View reviewed changes
lib/matplotlib/spines.py
Copy link
Copy Markdown
Member

@pelson pelson May 1, 2013

Choose a reason for hiding this comment

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

Thanks @cimarronm - this is much clearer now.

@pelson
Copy link
Copy Markdown
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.

👍

@cimarronm
Copy link
Copy Markdown
Contributor Author

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

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

@cimarronm
Copy link
Copy Markdown
Contributor Author

cimarronm commented May 10, 2013

Is this good to merge?

mdboom added a commit that referenced this pull request May 11, 2013
@mdboom
Merge pull request #1964 from cimarronm/data_spine_right_top_location…
af07100 
…_fix

Fixes issue #1960. Account for right/top spine data offset on transform ...
@mdboom mdboom merged commit af07100 into matplotlib:master May 11, 2013
@dmcdougall dmcdougall mentioned this pull request Jun 16, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cimarronm @pelson @mdboom
X Tutup