Python 3 compatibility fixes #369

jdeschenes · 2017-07-20T22:29:06Z

The unit test pass in python 3.6. This has not yet been completely tested in python. I will do another pass once I have gone over enable.

The examples have not been run as well. So I may make further modifications.

There is an issue in python 3 in the image plot. This is caused by a bug in enable. I skipped those tests.

* Mainly import problems.

* More fixes

* Preliminary commit... Several fixes were done.

# Conflicts: # chaco/api.py # chaco/axis.py # chaco/base.py # chaco/lineplot.py

# Conflicts: # chaco/jitterplot.py # chaco/text_plot_1d.py

# Conflicts: # chaco/plot.py

jvkersch · 2017-07-21T08:53:34Z

chaco/legend.py

@@ -355,7 +358,7 @@ def get_preferred_size(self):
        if len(self.plots) == 0:
            return [0, 0]

-        plot_names, visible_plots = map(list, zip(*sorted(self.plots.items())))
+        plot_names, visible_plots = list(sm.map(list, sm.zip(*sorted(six.iteritems(self.plots)))))


This could probably do with some unpacking. Alternatively, can we just use the standard Python constructs here and write the RHS as list(map(list, zip(*sorted(self.plots))))?

Your approach will not work. You need to have the items() in there

x = {'b': 1, 'a': 3} assert sorted(x) == ['a', 'b'] assert sorted(x.items()) == [('a', 3), ('b', 1)]

For standard python contructs. See my comment in enthought/traits#374

It's more or less the same issue. I would prefer to keep the iterator version whenever possible.

Let me know your decision.

Ah, I misunderstood. Thanks for the clarification, and let's keep it like you had it originally.

jvkersch · 2017-07-21T08:56:07Z

chaco/plotscrollbar.py

@@ -1,4 +1,6 @@



Can you put a from __future__ import print_function here?

jvkersch · 2017-07-21T08:59:00Z

chaco/scales/formatters.py

@@ -350,15 +354,19 @@ def estimate_width(self, start, end, numlabels=None, char_width=None,
            elif char_width:
                avg_size = len("%g%g" % (start, end)) / 2.0
                initial_estimate = round(fill_ratio * char_width / avg_size)
+            else:


This seems orthogonal to the cleanup, can you describe what the issue is here? If the issue is that numlabels and char_width should not be both None, perhaps a ValueError("num_labels and char_width should not both be None.") would be more appropriate.

jvkersch · 2017-07-21T08:59:33Z

chaco/scales/scales_test_case.py

@@ -3,8 +3,8 @@



Also needs a from __future__ import print_function.

jvkersch

@jdeschenes This is looking very good already, thanks for doing this! I left some general comments here and here, pointing out similar issues as @mdickinson reported for Traits. It also looks as if there are some changes that affect functionality, if possible it would be good to submit those separately (it will making reviewing easier). If there any areas (particularly with the demos) where the rest of the conversion is problematic, please let me know!

jvkersch · 2017-07-21T09:00:06Z

chaco/scales/tests/test_formatters.py

@@ -42,7 +42,7 @@ def test_strftimeEx_04():
    # The format "%(ms_)" uses floor().


Also needs a from __future__ import print_function at the top of the module.

jvkersch · 2017-07-21T09:01:55Z

chaco/scales/time_scale_test_case.py

@@ -2,11 +2,11 @@
 from itertools import starmap


Also needs a from __future__ import print_function at the top of the module.

jvkersch · 2017-07-21T09:03:22Z

chaco/tests/test_image_plot.py

@@ -108,48 +113,56 @@ def plot_comparison(input_image, expected_image, **plot_kwargs):
    plt.show()


+@unittest.skipIf(six.PY3, "Bug in the image plotter in python 3")


That's unexpected, is that new?

That's actually a pretty old enthought/enable#95(See also enthought/enable#283). There are probably more related issues on github. I have the same issues on Windows so it is not strictly a gcc or clang issue.

I see, can you add a reference to the issue in the skip message?

jvkersch · 2017-07-21T09:05:14Z

chaco/tools/line_inspector.py

@@ -78,7 +78,8 @@ def draw(self, gc, view_bounds=None):
        if self.is_listener:
            tmp = self._get_screen_pts()
        elif self.is_interactive:
-            tmp = self._last_position


It looks like this (and some of the other changes in this module) changes the functionality. Can you explain why this is necessary? If this fixes a bug, it would probably be good to split it off from the current PR and submit as a separate PR.

Hi, I pulled in my branch #205 as I found it helped quite a bit and it looked like it will not be implemented anytime soon. I reverted changes in latest commit.

jvkersch · 2017-07-21T09:05:43Z

chaco/tools/pan_tool.py

@@ -247,6 +247,8 @@ def panning_mouse_leave(self, event):
        return self._end_pan(event)

    def _start_pan(self, event, capture_mouse=True):
+        if event.control_down:


Also a functionality change.

Sorry about that... This has been reverted.

jvkersch · 2017-07-21T09:06:39Z

docs/source/sphinxext/comment_eater.py

@@ -1,10 +1,11 @@
-from cStringIO import StringIO


I think some of the bundled sphinx extensions in this folder might not be Python3 compatible... (Just a comment in passing, no need to take any action)

jvkersch · 2017-07-21T09:08:00Z

examples/demo/basic/image_lasso.py


        # We first map to screen because the selection is stored as coordinates
        # in data space
        screen_pts = lasso_tool.plot.map_screen(selection)

        # Now map each point into the grid index
-        for x, y in screen_pts:
-            print("\t", lasso_tool.plot.map_index((x, y)))
+        # for x, y in screen_pts:


Can you remove the commented-out code here?

jvkersch · 2017-07-21T09:08:44Z

examples/demo/image_plot_origin_and_orientation.py

@@ -13,7 +13,7 @@
 """

 # Major library imports
-from scipy.misc import lena
+from scipy.misc import face


jvkersch · 2017-07-21T09:09:06Z

examples/demo/multiaxis_using_Plot.py

+from chaco.tools.api import SelectedZoomState
+import numpy
+
+class CaeZoomTool(ZoomTool):


Sorry about that, this should not have been part of the commit.

No worries, looked like a pretty nifty tool ;-)

jdeschenes · 2017-07-21T11:04:04Z

Sorry about that, looks like I bundled more changes(I am using this branch) than strictly necessary. I will do a pass and remove functional changes.

* used `from __future__ import print_function` on all files with print functions * Removed usage of `six.u`. Python 3.2 will not be supported * Fixed an error message in formatters.py * Forgot a `end=" "` in scales_test_case.py * Removed a trailing backslash in plot_maker.py * Removed integration of changes in line_inspector.py * Removed changes from pan_tool.py * traitsdoc.py had wrong import format * Uncommented code in image_lasso.py * Uncommented code in financial_plot.py * Removed changes from multiaxis_using_Plot.py * Removed changes from two_plots.py

jdeschenes · 2017-07-21T13:54:11Z

I think I need to comment on some of the functional changes that I put there by mistake.

For multiaxis_using_Plot.py:

Using the zoom tool with ctrl is doing a pan at the same time(pan_tool.py.) Strangely, this doesn't happen in two_plots.py. I think a code change may be necessary here. My approach was to subclass PanTool and ignore the pan move if my modifiers were pressed.
The Box on the ZoomTool does not appear
The ZoomTool is not doing the correct zoom(See issue ZoomTool doesn't play well with secondary axis #366) The CaeZoomTool has this fixed, but only partially... Using the mouse wheel to zoom off will not do what is right.

For financial_plot.py

The font does not render properly on my machine. There is a warning on font_manager.py in kiva. That's a complete separate issue. Here is the related kiva/fonttools/fontmanager.py:findfont unable to find suitable fonts enable#259
These changes definitively don't belong in this pull request.

jvkersch · 2017-07-23T15:25:58Z

@jdeschenes Thanks for addressing the comments. Would you mind merging master in and resolving any merge conflicts? It would be good to start getting Travis test results on this.

# Conflicts: # chaco/api.py

jdeschenes · 2017-07-23T16:19:55Z

The merge has just been done.

jvkersch · 2017-07-25T09:17:32Z

chaco/speedups.py

 except ImportError:
+    # TO TEST
+    raise


Do we need this explicit raise?

jvkersch · 2017-07-25T09:19:16Z

docs/source/conf.py

@@ -52,7 +52,9 @@ def get_build_docset():
 # The default replacements for |version| and |release|, also used in various
 # other places throughout the built documents.
 d = {}
-execfile(os.path.join('..', '..', 'chaco', '_version.py'), d)
+exec(compile(open(os.path.join('..', '..', 'chaco', '_version.py')).read(),


Can you isolate the os.path.join('..', '..', 'chaco', '_version.py') into a local variable for re-use?

jvkersch · 2017-07-25T09:20:35Z

@jdeschenes I left two tiny comments, but otherwise this looks very good to me. If you fix up those two items (and have nothing more to add), then I think this is good to go.

jdeschenes · 2017-07-25T15:45:58Z

@jvkersch I fixed the issues you mentionned. I think this is ready to go!

jvkersch · 2017-07-27T07:24:30Z

@jdeschenes I'm following along with the Enable review, and I think it would be good here too to use .items(), .values(), etc. instead of six.iteritems(...), six.itervalues(...), and so on. Would you be able to make that change? If not, I will pick this up separately (and apologies for bringing this up at the last minute).

jdeschenes · 2017-08-02T21:41:18Z

@jvkersch I think I have corrected everything. Let me know if there is anything else!

jvkersch · 2017-08-03T08:36:37Z

@jdeschenes I think this looks good to go!

Thanks a lot for doing this (and for tackling the other repos, too). This will improve developer experience by a lot!

Introduced in #369 Fixes #393

jmdeschenes added 11 commits September 9, 2016 14:28

Python 3 compatibility fixes

788257c

* Mainly import problems.

Python 3 compatibility fixes

99d179b

* More fixes

Python 3 compatibility fixes

dc08831

* Preliminary commit... Several fixes were done.

Python 3 compatibility fixed

2db1e5b

Fix for text_plot_1d

c41ea42

Merge remote-tracking branch 'source/master'

5d8aaf7

# Conflicts: # chaco/api.py # chaco/axis.py # chaco/base.py # chaco/lineplot.py

Merge branch 'master' into python-3

1364a9f

# Conflicts: # chaco/jitterplot.py # chaco/text_plot_1d.py

Additional changes to make things work on python35

f2b83a2

Merge branch 'master' of https://github.com/enthought/chaco

dcaab2d

# Conflicts: # chaco/plot.py

Merge branch 'master' into python-3

d3c41c2

Unit tests all pass.

47cff1d

jvkersch reviewed Jul 21, 2017

View reviewed changes

chaco/plotscrollbar.py

@@ -1,4 +1,6 @@

Copy link

Contributor

jvkersch Jul 21, 2017

Choose a reason for hiding this comment

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

Can you put a from __future__ import print_function here?

jvkersch reviewed Jul 21, 2017

View reviewed changes

chaco/scales/scales_test_case.py

@@ -3,8 +3,8 @@

Copy link

Contributor

jvkersch Jul 21, 2017

Choose a reason for hiding this comment

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

Also needs a from __future__ import print_function.

jvkersch reviewed Jul 21, 2017

View reviewed changes

Merge branch 'master' into python-3

9317a52

# Conflicts: # chaco/api.py

Added the Github issue of enable that prevents image plots from working

f0efb40

jvkersch reviewed Jul 25, 2017

View reviewed changes

chaco/speedups.py Outdated

except ImportError:

# TO TEST

raise

Copy link

Contributor

jvkersch Jul 25, 2017

Choose a reason for hiding this comment

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

Do we need this explicit raise?

jvkersch reviewed Jul 25, 2017

View reviewed changes

Correction from comment of @jvkersch

95298a7

Corrected the six.iteritems, itervalues and iterkeys

3655a79

jvkersch merged commit 7c61319 into enthought:master Aug 3, 2017

jvkersch mentioned this pull request Oct 31, 2017

MAINT: Get docs to build again #384

Merged

rkern added a commit that referenced this pull request Dec 18, 2017

BUG: Fix use of itertools.chain()

8f59ecc

Introduced in #369 Fixes #393

rkern mentioned this pull request Dec 18, 2017

BUG: Fix use of itertools.chain() #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3 compatibility fixes #369

Python 3 compatibility fixes #369

jdeschenes commented Jul 20, 2017

jvkersch Jul 21, 2017

jdeschenes Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch left a comment

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jdeschenes Jul 21, 2017 •

edited

Loading

jvkersch Jul 23, 2017

jvkersch Jul 21, 2017

jdeschenes Jul 21, 2017

jvkersch Jul 21, 2017

jdeschenes Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jvkersch Jul 21, 2017

jdeschenes Jul 21, 2017

jvkersch Jul 21, 2017

jdeschenes commented Jul 21, 2017

jdeschenes commented Jul 21, 2017 •

edited

Loading

jvkersch commented Jul 23, 2017

jdeschenes commented Jul 23, 2017

jvkersch Jul 25, 2017

jvkersch Jul 25, 2017

jvkersch commented Jul 25, 2017

jdeschenes commented Jul 25, 2017

jvkersch commented Jul 27, 2017

jdeschenes commented Aug 2, 2017

jvkersch commented Aug 3, 2017

		@@ -42,7 +42,7 @@ def test_strftimeEx_04():
		# The format "%(ms_)" uses floor().

		@@ -108,48 +113,56 @@ def plot_comparison(input_image, expected_image, **plot_kwargs):
		plt.show()


		@unittest.skipIf(six.PY3, "Bug in the image plotter in python 3")

Python 3 compatibility fixes #369

Python 3 compatibility fixes #369

Conversation

jdeschenes commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvkersch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdeschenes Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdeschenes commented Jul 21, 2017

jdeschenes commented Jul 21, 2017 • edited Loading

jvkersch commented Jul 23, 2017

jdeschenes commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvkersch commented Jul 25, 2017

jdeschenes commented Jul 25, 2017

jvkersch commented Jul 27, 2017

jdeschenes commented Aug 2, 2017

jvkersch commented Aug 3, 2017

jdeschenes Jul 21, 2017 •

edited

Loading

jdeschenes commented Jul 21, 2017 •

edited

Loading