search

bastula / dicompyler

Extensible radiation therapy research platform and viewer for DICOM and DICOM RT
http://www.dicompyler.com
263 stars 99 forks source link

Python3 update #103

Closed StephenTerry closed 7 years ago

StephenTerry commented 7 years ago

I made some changes to get dicompyler working in Python 3.6. It requires wxpython phoenix 4.0.0a3 or higher because of this bug with subclasses in the XRC file. This code works with the test data on Windows, but I haven't tested it on Linux or MacOS. I'm still working on some crashes I'm getting with other DICOM data. There's a corresponding pull request for dicompyler-core for some minor changes to dvhcalc.py to get the main program to work in Python 3.

bastula commented 7 years ago

Thanks for this comprehensive PR. I was wondering should dicompyler move fully to Python 3 or is it worth it to keep Python 2.7 compatibility around? I tried to maintain that with dicompyler-core.

I know pylinac is Python 3 only and it hasn't slowed adoption afaik.

I personally am not tied to Python 2.7 but I figured with my previous commit it would provide a migration path for users who want to stay on Python 2 and just update library dependencies. We could tag that commit as the last supported Python 2 codebase.

If Python 3 is the way forward I wouldn't be opposed to moving this to master sooner than later.

Thoughts?

StephenTerry commented 7 years ago

It's a tough one. I think it might not be that much more work to keep two active branches going. I think the big question is what plugin authors want to do. Maintaining a plugin for two different versions might be more work than they want to do.

You could try making both versions available and seeing which one gets downloaded the most. There's still a long way to go on the 3.6 version, so you may not need to make a decision now.

bastula commented 7 years ago

I think I am more inclined to go Python 3 all the way. I think if people really have a need, dicompyler-core can stay Py2/3, as that seems to be more of a use case as it is a library.

It's more work to release multiple versions, but I think a separate discussion should be automating releases via Travis/AppVeyor instead of the old manual build process.

StephenTerry commented 7 years ago

That's fine with me. I'm not very fluent with wxPython, so I was leaving them in just in case, but of course you're right that we can refer to previous commits.