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 python3 issues in some examples #3831

Merged
merged 13 commits into from Jan 1, 2015

Conversation

jenshnielsen
Copy link
Member

Unfortunately these are the easy ones ...

This mostly involves intervals/keys and xrange. For the most part I chose simplicity over minimising memory usage and replaced interkeys etc. with keys. The loops are rather short so the benefits from iterators is rather limited. If anyone thinks it is important I am happy to change this.

@@ -16,7 +17,7 @@

ax.text(2, 6, r'an equation: $E=mc^2$', fontsize=15)

ax.text(3, 2, unicode('unicode: Institut f\374r Festk\366rperphysik', 'latin-1'))
ax.text(3, 2, u('unicode: Institut f\374r Festk\366rperphysik'))
Copy link
Member

Choose a reason for hiding this comment

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

haven't seen this one before from six. Which version was it added?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are already using it in some other places. If we are willing to drop support for python 3.2 we could actually drop it and replace it with regular u'string'

Deleted my previous comment since it was wrong and made no sense

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought matplotlib had already dropped 3.2 support? Certainly seems like a good idea to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't test it but afaik we haven't actually on purpose introduced code that breaks support. In any case this is only an example so much less important than core code to remain compatible with old versions

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and also the example should be as readable and idiomatic as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to use u'string' then

@jenshnielsen
Copy link
Member Author

@WeatherGod Actually there is a bigger issue with marker_reference.py the previous implementation relied on the std python2 sorting of ints before strings. This version converts everything to string before sorting which puts the int next to the matching string. I can not find a build in way to restore python2 behaviour in python3 so this seemed like the simplest solution. We can easily reimplement python 2 sorting but I don't think the extra code it worth it in an example. It will only distract from the subject of the example.

@@ -4,7 +4,7 @@


def adjust_spines(ax,spines):
for loc, spine in ax.spines.iteritems():
for loc, spine in ax.spines.items():
Copy link
Member

Choose a reason for hiding this comment

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

This should be done as six.iteritems(spines)

Copy link
Member Author

Choose a reason for hiding this comment

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

I deliberately chose to avoid six as much as possible to simplify the examples. As I see it examples has a different balance between code readability and performance. But you are probably right that in the cases of axes spines etc. which are somewhat longer loops it makes sense to use iterators.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. At this point I think of six as being part of the 'standard'
library and don't write anything without it so did not give it a tremendous
amount of thought.

The case could be made that the examples are should also be examples of
general best practices, and python 2/3 compatibility is one of them, but I
will defer to your judgement on this.

On Tue Nov 25 2014 at 3:40:52 PM Jens Hedegaard Nielsen <
notifications@github.com> wrote:

In doc/pyplots/whats_new_99_spines.py:

@@ -4,7 +4,7 @@

def adjust_spines(ax,spines):

  • for loc, spine in ax.spines.iteritems():
  • for loc, spine in ax.spines.items():

I deliberately chose to avoid six as much as possible to simplify the
examples. As I see it examples has a different balance between code
readability and performance. But you are probably right that in the cases
of axes spines etc. which are somewhat longer loops it makes sense to use
iterators.


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3831/files#r20893607.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will go through them one by one and change to six in cases like this where it is a definite advantage.

Copy link
Member

Choose a reason for hiding this comment

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

Use your judgment; in my view, six should be used as little as possible, especially in examples. It's an ugly but sometimes necessary workaround for 2/3 compatibility. One would have to have a huge dictionary before there would be any practical difference between items and iteritems, for example.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @efiring here: Let's not use six in examples unless there is a strong reason on a case-by-case basis.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only 4 spines in a figure so six is unlikely to make a difference.

@tacaswell tacaswell added this to the v1.5.x milestone Nov 25, 2014
@@ -23,7 +23,7 @@ def squiggle_xy(a, b, c, d, i=np.arange(0.0, 2*np.pi, 0.05)):
# gridspec inside gridspec
outer_grid = gridspec.GridSpec(4, 4, wspace=0.0, hspace=0.0)

for i in xrange(16):
for i in range(16):
Copy link
Member Author

Choose a reason for hiding this comment

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

16 elements. Iterator makes no difference

@jenshnielsen
Copy link
Member Author

Finally got round to looking at this again. Rebased and changed the one where I think it makes sense to an iterator. Note the issue above relating to sort order. If anyone has a good idea of a simple solution to that I am happy to implement it.

@tacaswell
Copy link
Member

I think something like

In [4]: sorted((1, 2, 3, '1', '2', '3'), key=lambda x: (str(type(x)), str(x)) )
Out[4]: [1, 2, 3, '1', '2', '3']

would work and isn't too ugly looking

@jenshnielsen
Copy link
Member Author

@tacaswell Thats sensible. Done

tacaswell added a commit that referenced this pull request Jan 1, 2015
DOC : Fix python3 issues in some examples
@tacaswell tacaswell merged commit d8b2924 into matplotlib:master Jan 1, 2015
@jenshnielsen jenshnielsen deleted the examples_python3_fixup branch January 1, 2015 18:14
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

6 participants