CellProfiler / centrosome

An open source image processing library
Other
18 stars 35 forks source link

Add support for Python 3 #84

Closed jni closed 6 years ago

jni commented 6 years ago

Closes #19

There were two things I wasn't able to fix:

If someone could review this I would much appreciate it!

jni commented 6 years ago

@0x00b1 @mcquin can I just get an acknowledgement from someone? =) I just want to make sure someone knows this PR exists.

AetherUnbound commented 6 years ago

My greatest concern is backwards compatibility, since changing all of CellProfiler to Python 3 will be a large undertaking. It looks like all the tests (but bilateral, as you described) is passing! @0x00b1 may have to look into that a bit more. This LGTM, but I want to defer to Allen for merges on this repo.

The only other thing I would say is 2to3 adds list castings around generators that don't necessarily need to be converted to lists. They're sprinkled throughout your repo so I didn't want to comment on every one, but many of those are likely unnecessary.

Thanks for your work! This is an awesome set of changes ๐ŸŽ‰

jni commented 6 years ago

Ok, this should be really ready now except for the bilateral filter test. @0x00b1 can you investigate that one?

@AetherUnbound et al, yes, the output of modernize on the CP repo is eye-watering. ๐Ÿ˜‚ ๐Ÿ˜ญ ๐Ÿ˜‚ Nevertheless, we want to expand CellProfiler use at my new group at Monash Micro Imaging so I anticipate a lot of work on the source. My OCD prevents me from working on a Python 2-only codebase without at least attempting an update, and anyway I would expect that CP will continue to be used well past 2020 so this work needs to happen at some point. So, I'm gonna give it a go, once this is merged. Wish me luck! ๐Ÿ˜‚

0x00b1 commented 6 years ago

The only other thing I would say is 2to3 adds list castings around generators that don't necessarily need to be converted to lists. They're sprinkled throughout your repo so I didn't want to comment on every one, but many of those are likely unnecessary.

Yeah, I was curious about that too. Nevertheless, it doesnโ€™t really bother me. Do you have a preference @AetherUnbound?

0x00b1 commented 6 years ago

Ok, this should be really ready now except for the bilateral filter test. @0x00b1 can you investigate that one?

Yep! No problem! @bethac07 and I are looking into this now. I really appreciate the PR!

0x00b1 commented 6 years ago

@jni @AetherUnbound: @bethac07 and I deprecated bilateral_filter for skimage.restoration.denoise_bilateral:

https://github.com/jni/centrosome/pull/1

jni commented 6 years ago

@0x00b1

Yeah, I was curious about that too. Nevertheless, it doesnโ€™t really bother me. Do you have a preference @AetherUnbound?

I think I removed all of the unnecessary casts in 4dd28fc anyway...

jni commented 6 years ago

Hmm, bilateral filter is failing in Py2.7 anyway... Maybe it's a NumPy change more than a Python version change...?

jni commented 6 years ago

@AetherUnbound @0x00b1 care to take another look at merging this?

jni commented 6 years ago

ping @AetherUnbound @0x00b1

jni commented 6 years ago

ping some more! =P Just fyi I'm back from SciPyConf and would like to start hacking at CellProfiler sometime next week.

AetherUnbound commented 6 years ago

Gosh, sorry @jni! I have to find a better way to filter notifications for Github. This looks good to me, let me check in with @0x00b1 on Monday and we'll see if we can merge this!

jni commented 6 years ago

:tada: