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

position of text annotations looses unit information #4904

Merged
merged 2 commits into from Aug 22, 2015

Conversation

jrevans
Copy link

@jrevans jrevans commented Aug 11, 2015

A person could set the position using units, but when getting them back, the units would be stripped away. This made no sense since the unitized data was still present and the units were only being stripped away for use in internal methods. This fixes the behavior so the user will get back exactly what they set and the internal routines will still get the unitless data as needed.

This addresses an issue in #4897.

after it was set.  This fixes Text such that it will return the original data when
asked and will only strip away the units when it is needed.
@tacaswell tacaswell added this to the next point release milestone Aug 11, 2015
@tacaswell
Copy link
Member

From git blame on this section of the code it looks like stripping the units has been the behavior since a43661e in from 2007.

I agree this should go in, but could you please add an api_changes entry?

It might be worth making _get_unitless_postion public to give user who are relying on this behavior a public way to access the old behavior.

@jrevans
Copy link
Author

jrevans commented Aug 17, 2015

I can change '_get_unitless_position' to be public if that is what you want. I am not sure what you mean by add an 'api_changes' entry.

@tacaswell
Copy link
Member

See matplotlib/doc/api/api_changes/README

On Mon, Aug 17, 2015, 3:55 PM James Evans notifications@github.com wrote:

I can change '_get_unitless_position' to be public if that is what you
want. I am not sure what you mean by add an 'api_changes' entry.


Reply to this email directly or view it on GitHub
#4904 (comment)
.

@WeatherGod
Copy link
Member

He is referring to "doc/api/api_changes/".

On Mon, Aug 17, 2015 at 3:55 PM, James Evans notifications@github.com
wrote:

I can change '_get_unitless_position' to be public if that is what you
want. I am not sure what you mean by add an 'api_changes' entry.


Reply to this email directly or view it on GitHub
#4904 (comment)
.

Renamed '_get_unitless_position' to 'get_unitless_position'
@tacaswell
Copy link
Member

All of the test failures look like they were due to the network issues gh was having when they ran (all failed out on trying to hit a url for the style module or known transient failures). Kicked travis to restart ,will merge as soon as it comes back green.

I am 👍 on this API change because it only effects people who are using units and the behavior they expect is what it has been changed to.

@jrevans Could you follow this up with a test? I think just showing values round-trip with units is enough.

tacaswell added a commit that referenced this pull request Aug 22, 2015
API : Text.get_position now preserves units
@tacaswell tacaswell merged commit 72b0a92 into matplotlib:master Aug 22, 2015
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

3 participants