biorack / omaat

Other
4 stars 2 forks source link

make python 2 and 3 compatible #1

Closed benbowen closed 8 years ago

benbowen commented 8 years ago

Since people will be using this code for a long time, I want to go ahead and make it 2 and 3 cros-compatible.

This generally means writing it for Python 3 and making reverse imports that allow it to work in 2.7.

I created a new .ipy file so it doesn't mess up our production version. Its mainly print statements and string decoding. Feel free to occasionally hack away at this.

I'm testing with a dual environment setup this way: http://conda.pydata.org/docs/py2or3.html

aitatanit commented 8 years ago

Will do

On Wed, Jun 8, 2016 at 12:29 PM Ben Bowen notifications@github.com wrote:

Since people will be using this code for a long time, I want to go ahead and make it 2 and 3 cros-compatible.

This generally means writing it for Python 3 and making reverse imports that allow it to work in 2.7.

I created a new .ipy file so it doesn't mess up our production version. Its mainly print statements and string decoding. Feel free to occasionally hack away at this.

I'm testing with a dual environment setup this way: http://conda.pydata.org/docs/py2or3.html

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1, or mute the thread https://github.com/notifications/unsubscribe/ACbTPQk-Cls_f5Tgoo5OlIUByJw2Fom9ks5qJvv6gaJpZM4IxNPO .

benbowen commented 8 years ago

@aitatanit, this is actually not a metatlas project, but feel free to help out with it. :) These arrayed analysis tools are mainly written by @tderond and me with help from @raad0102.

tderond commented 8 years ago

hopefully will take a crack at it tomorrow.

Tristan ⋈

On Wed, Jun 8, 2016 at 10:34 AM, Ben Bowen notifications@github.com wrote:

@aitatanit https://github.com/aitatanit, this is actually not a metatlas project, but feel free to help out with it. :) These arrayed analysis tools are mainly written by @tderond https://github.com/tderond and me with help from @raad0102 https://github.com/raad0102.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-224668510, or mute the thread https://github.com/notifications/unsubscribe/AMuAgStcPlV1EkIk2g0LrGLCUmJ1WHNhks5qJv07gaJpZM4IxNPO .

benbowen commented 8 years ago

@tderond I regretfully made a copy of the .ipy file, but the "git" way would have been to make a branch to develop for this issue.

tderond commented 8 years ago

ran the futurize script on the code (needed to take out the ipython "magic" first, because the futurize script didnt like that), which generated this diff. hopefully after i apply all those changes to the code, it'll be cross-compatible.

tderond commented 8 years ago

I ended up applying most of the changes in the diff file, except for: -replacing input() by eval(input()). I noticed @oruebel had already replaced input() by raw_input(), but instead using "from builtins import input" is cross-compatible) -python2-style division. Hopefully python3 division (which is always floating point) will fix the rounding error @raad0102 brought up.

testing now before I commit

tderond commented 8 years ago

Alright it works in both python 2 and 3 for me. please help beta-best.

raad0102 commented 8 years ago

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell screen shot 2016-06-20 at 11 03 20 am

oruebel commented 8 years ago

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 notifications@github.com wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123, or mute the thread https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO .

tderond commented 8 years ago

Oliver, do you mean I should have used a different package for the builtins?

Tristan ⋈

On Mon, Jun 20, 2016 at 11:08 AM, Oliver Ruebel notifications@github.com wrote:

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 notifications@github.com wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] < https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123 , or mute the thread < https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227222029, or mute the thread https://github.com/notifications/unsubscribe/AMuAgaBf4zWCKxO2659zTL4Z6OscTb09ks5qNtcMgaJpZM4IxNPO .

oruebel commented 8 years ago

You could do something like this:

import sys if sys.version_info >= (3,): from builtins import zip from builtins import chr from builtins import str else: from builtin import zip from builtin import chr from builtin import str

Alternatively you could also use the future package:

https://pypi.python.org/pypi/future

which might make things a bit easier but requires you to have an additional library.

Cheers,

Oliver

On Mon, Jun 20, 2016 at 11:39 AM, Tristan notifications@github.com wrote:

Oliver, do you mean I should have used a different package for the builtins?

Tristan ⋈

On Mon, Jun 20, 2016 at 11:08 AM, Oliver Ruebel notifications@github.com wrote:

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 notifications@github.com wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] <

https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123

, or mute the thread <

https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub < https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227222029 , or mute the thread < https://github.com/notifications/unsubscribe/AMuAgaBf4zWCKxO2659zTL4Z6OscTb09ks5qNtcMgaJpZM4IxNPO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227230844, or mute the thread https://github.com/notifications/unsubscribe/AKfYJYjg3V9W3v4hplluk3fkt3U1ger9ks5qNt5mgaJpZM4IxNPO .

tderond commented 8 years ago

I'm just confused because this code worked fine on Python 2 on my computer. so I must have the python 2 package "builtins" installed. Now, does requiring people to have "builtins" installed cause more trouble than requireing them to have both "future" and "builtins" installed?

I don't think the builtin package contains the functions i'm trying to import, and those it does contain i dont think have python3 behavior.

This hurts my head why do all these packages have almost the same name

Tristan ⋈

On Mon, Jun 20, 2016 at 12:04 PM, Oliver Ruebel notifications@github.com wrote:

You could do something like this:

import sys if sys.version_info >= (3,): from builtins import zip from builtins import chr from builtins import str else: from builtin import zip from builtin import chr from builtin import str

Alternatively you could also use the future package:

https://pypi.python.org/pypi/future

which might make things a bit easier but requires you to have an additional library.

Cheers,

Oliver

On Mon, Jun 20, 2016 at 11:39 AM, Tristan notifications@github.com wrote:

Oliver, do you mean I should have used a different package for the builtins?

Tristan ⋈

On Mon, Jun 20, 2016 at 11:08 AM, Oliver Ruebel < notifications@github.com> wrote:

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 notifications@github.com wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] <

https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123

, or mute the thread <

https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227222029

, or mute the thread <

https://github.com/notifications/unsubscribe/AMuAgaBf4zWCKxO2659zTL4Z6OscTb09ks5qNtcMgaJpZM4IxNPO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227230844 , or mute the thread < https://github.com/notifications/unsubscribe/AKfYJYjg3V9W3v4hplluk3fkt3U1ger9ks5qNt5mgaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227237853, or mute the thread https://github.com/notifications/unsubscribe/AMuAgRk60AfYgUooeal6po75f8YVHbQUks5qNuRFgaJpZM4IxNPO .

tderond commented 8 years ago

Apparently both "future" and "builtins" are in the package "future", so there's no reason to use one and not the other. I don't think there's a way around wanting cross-compatible code without the "future" package.

So, @raad0102, could you please try installing the "future" package ("conda install future") and try running again?

Tristan ⋈

On Mon, Jun 20, 2016 at 1:33 PM, Tristan de Rond tristan.de.rond@gmail.com wrote:

I'm just confused because this code worked fine on Python 2 on my computer. so I must have the python 2 package "builtins" installed. Now, does requiring people to have "builtins" installed cause more trouble than requireing them to have both "future" and "builtins" installed?

I don't think the builtin package contains the functions i'm trying to import, and those it does contain i dont think have python3 behavior.

This hurts my head why do all these packages have almost the same name

Tristan ⋈

On Mon, Jun 20, 2016 at 12:04 PM, Oliver Ruebel notifications@github.com wrote:

You could do something like this:

import sys if sys.version_info >= (3,): from builtins import zip from builtins import chr from builtins import str else: from builtin import zip from builtin import chr from builtin import str

Alternatively you could also use the future package:

https://pypi.python.org/pypi/future

which might make things a bit easier but requires you to have an additional library.

Cheers,

Oliver

On Mon, Jun 20, 2016 at 11:39 AM, Tristan notifications@github.com wrote:

Oliver, do you mean I should have used a different package for the builtins?

Tristan ⋈

On Mon, Jun 20, 2016 at 11:08 AM, Oliver Ruebel < notifications@github.com> wrote:

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 notifications@github.com wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] <

https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123

, or mute the thread <

https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227222029

, or mute the thread <

https://github.com/notifications/unsubscribe/AMuAgaBf4zWCKxO2659zTL4Z6OscTb09ks5qNtcMgaJpZM4IxNPO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227230844 , or mute the thread < https://github.com/notifications/unsubscribe/AKfYJYjg3V9W3v4hplluk3fkt3U1ger9ks5qNt5mgaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227237853, or mute the thread https://github.com/notifications/unsubscribe/AMuAgRk60AfYgUooeal6po75f8YVHbQUks5qNuRFgaJpZM4IxNPO .

raad0102 commented 8 years ago

After installing future, it now runs

oruebel commented 8 years ago

Just to clarify, the builtins module under Python 3 and the builtin (no s) module in Python 2 should be the same. The builtin module was renamed in Python 3 to builtins to avoid the confusion that exists in Python 2 between builtin and builtins (with and without s, both exists in Python 2 and have slightly different meaning). However, the way the notebook works right now, it seems that you actually need to have the builtins module for things to work. Somehow, there seems to be a collision in the iPython kernel when you call the input function when you use the default. My guess is that probably the ipywidgets overwrite the input function. I don't think that requiring the future module should be a big problem so I would just go with that.

Just for my own understanding, why do you need to import the function from builtin explicitly here?

Cheers,

Oliver

On Mon, Jun 20, 2016 at 1:34 PM, Tristan notifications@github.com wrote:

I'm just confused because this code worked fine on Python 2 on my computer. so I must have the python 2 package "builtins" installed. Now, does requiring people to have "builtins" installed cause more trouble than requireing them to have both "future" and "builtins" installed?

I don't think the builtin package contains the functions i'm trying to import, and those it does contain i dont think have python3 behavior.

This hurts my head why do all these packages have almost the same name

Tristan ⋈

On Mon, Jun 20, 2016 at 12:04 PM, Oliver Ruebel notifications@github.com wrote:

You could do something like this:

import sys if sys.version_info >= (3,): from builtins import zip from builtins import chr from builtins import str else: from builtin import zip from builtin import chr from builtin import str

Alternatively you could also use the future package:

https://pypi.python.org/pypi/future

which might make things a bit easier but requires you to have an additional library.

Cheers,

Oliver

On Mon, Jun 20, 2016 at 11:39 AM, Tristan notifications@github.com wrote:

Oliver, do you mean I should have used a different package for the builtins?

Tristan ⋈

On Mon, Jun 20, 2016 at 11:08 AM, Oliver Ruebel < notifications@github.com> wrote:

Note, there is a future builtins module for this:

https://docs.python.org/2/library/future_builtins.html

On Mon, Jun 20, 2016 at 11:04 AM, raad0102 <notifications@github.com

wrote:

I just pulled the new code and tried to run it using Python 2.7 but got this error in the first cell [image: screen shot 2016-06-20 at 11 03 20 am] <

https://cloud.githubusercontent.com/assets/10214084/16205054/c13889dc-36d6-11e6-8e01-8e8908378caf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227221123

, or mute the thread <

https://github.com/notifications/unsubscribe/AKfYJSajGtAiYvYsRGhFJwWrN3CXSXjKks5qNtY7gaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227222029

, or mute the thread <

https://github.com/notifications/unsubscribe/AMuAgaBf4zWCKxO2659zTL4Z6OscTb09ks5qNtcMgaJpZM4IxNPO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227230844

, or mute the thread <

https://github.com/notifications/unsubscribe/AKfYJYjg3V9W3v4hplluk3fkt3U1ger9ks5qNt5mgaJpZM4IxNPO

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub < https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227237853 , or mute the thread < https://github.com/notifications/unsubscribe/AMuAgRk60AfYgUooeal6po75f8YVHbQUks5qNuRFgaJpZM4IxNPO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227260965, or mute the thread https://github.com/notifications/unsubscribe/AKfYJQK9T25Un89k7skWghc1FlVinILtks5qNvlBgaJpZM4IxNPO .

tderond commented 8 years ago

The short answer is: because "futurize" told me to import those builtins

The long answer: some builtins have different meanings in python2 vs python3. the builtin module allows you to use python3 behaviour in python2, which makes it cross-compatible. (http://python-future.org/imports.html)

I'm gonna look into the "input" issue tomorrow hopefully. I think you're right that ipython/jupyer overwrites the input and raw_input functions (to allow them to ask for input in the notebook rather than in the terminal). Does this actually affect the way the notebook runs right now?

oruebel commented 8 years ago

When you use the builtins module everything seems to work fine. However, when I tried to use Python 2 version of the functions, then things crashed. If you require the future package and use builtins it seems like things work fine.

Cheers,

Oliver

On Mon, Jun 20, 2016 at 3:18 PM, Tristan notifications@github.com wrote:

The short answer is: because "futurize" told me to import those builtins

The long answer: some builtins have different meanings in python2 vs python3. the builtin module allows you to use python3 behaviour in python2, which makes it cross-compatible. (http://python-future.org/imports.html)

I'm gonna look into the "input" issue tomorrow hopefully. I think you're right that ipython/jupyer overwrites the input and raw_input functions (to allow them to ask for input in the notebook rather than in the terminal). Does this actually affect the way the notebook runs right now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biorack/OpenMSI_Arrayed_Analysis_Tools/issues/1#issuecomment-227287217, or mute the thread https://github.com/notifications/unsubscribe/AKfYJYOGaeDbG0KEhg2MCEfKjXw8hcQNks5qNxHRgaJpZM4IxNPO .