bastula / dicompyler

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

2D View plugin causes traceback with matplotlib 3.0.2 #122

Closed alanphys closed 4 years ago

alanphys commented 5 years ago

Dicomplyler ver 0.50 with matplotlib 3.0.2 gives the following traceback:

ERROR: 2dview could not be loaded Traceback (most recent call last): File "/home/alanphys/Downloads/SVN/dicompyler/dicompyler/plugin.py", line 51, in import_plugins m = imp.load_module(module, f, filename, description) File "/usr/lib64/python3.6/imp.py", line 235, in load_module return load_source(name, filename, file) File "/usr/lib64/python3.6/imp.py", line 172, in load_source module = _load(spec) File "", line 684, in _load File "", line 665, in _load_unlocked File "", line 678, in exec_module File "", line 219, in _call_with_frames_removed File "/home/alanphys/Downloads/SVN/dicompyler/dicompyler/baseplugins/2dview.py", line 14, in from matplotlib import _cntr as cntr ImportError: cannot import name '_cntr'

_cntr is apparently a private module and is no longer accessible from matplotlib

Workaround install legacycontour (https://github.com/matplotlib/legacycontour) change line 14 of 2dview.py to from legacycontour import _cntr as cntr

Tested on [GCC 8.3.1 20190223 (Red Hat 8.3.1-2)] on linux Type "help", "copyright", "credits" or "license" for more information.

import platform; print(platform.platform()) Linux-4.19.16-200.fc28.x86_64-x86_64-with-fedora-28-Twenty_Eight import sys; print("Python",sys.version) Python 3.6.8 (default, Mar 21 2019, 10:08:12) [GCC 8.3.1 20190223 (Red Hat 8.3.1-2)] import matplotlib; print("matplotlib",matplotlib.version) matplotlib 3.0.2

Regards Alan

bastula commented 5 years ago

Thanks for submitting this. I ran into this issue as well and considered using legacycontour or measure.findcontours from scikit-image.

Obviously legacycontour would be fine but you must have a C compiler as there probably are no compiled builds. scikit-image has pre-built wheels that will work on Linux/Mac/Windows.

Secondarily, measure.findcontours produces a different set of isocontours compared with legacycontour/ prior versions of matplotlib.

Another option, which I am not a big fan of is to set an upper bound on the version of matplotlib that could be used with dicompyler. This version would be the most recent one that still bundled _cntr.

alanphys commented 5 years ago

Yes, I had to install a couple of libraries to get it to compile. Is it not possible to have the developers distribute compiled versions on PyPi? Then you can specify it as a dependency along with matplotlib.

bastula commented 5 years ago

Well I think legacycontour is considered unsupported now. If we want compiled versions we would have to pin the matplotlib version lower or end up distributing it ourselves. The latter is something I'm not sure if dicompyler should be responsible for.

sjswerdloff commented 4 years ago

while you aren't a big fan of it (as you mention above), perhaps changing the requirements.txt to use is the thing to do until a better solution presents itself: matplotlib>=1.3,<2.2

from what I have seen in various complaints, the breaking change occurred in 2.2 (_cntr was dropped), and 2.1 was the last version reported that had _cntr in it. I've already walked through how to get legacycontour as an alternative into the requirements.txt (on MacOS) elsewhere.

I encourage a temporary solution that results in the code/build not being broken. I think it is better to have an imperfect (and hopefully temporary) solution than a broken build. Constraining matplotlib>=1.3,<2.2 ensures that a virtual environment for dicompyler will work without forcing the user/programmer to have a C/C++ compiler.

measure.findcontours produces a different set of isocontours

does that mean it is wrong? getting different results would be a bit disturbing, but with a new version of dicompyler that documents the change, it eliminates the obsolescence problem. getting "wrong" results is a different story (and might warrant an issue and work on measure.findcontours, perhaps mining the code in legacycontours to understand a correct solution).

bastula commented 4 years ago

I think either approach is valid, but the first is preferred due to maintaining existing library requirements (just pinning the upper version limit for matplotlib).

I personally have now used measure.find_contours from scikit-image and have found it satisfactory for general use in another project. I kind of want to stay away from this for now, just because it makes the project more heavyweight, and introducing another dependency just for 1 function isn't especially ideal.

In the future, a condition could be put in to detect the version of matplotlib and if greater than 2.1, prompt the user to install scikit-image.

I will update requirements.txt and setup.py to include the upper bound of 2.1 for matplotlib.

sandrotosi commented 4 years ago

I think this is a mistake: matplotlib < 2.2 means you're forcing to install mpl 2.1.8, which is 2 years old already.

For example in Debian, we have matplotlib 3.1.2: should we install matplotlib and scikit-image?

thanks!

bastula commented 4 years ago

That is true, but as @sjswerdloff mentioned, this way the application still works for those that are able to use that version. The work to swap in scikit-image for matplotlib has not been implemented.

jmartens commented 2 years ago

I propose to reopen this issue and address this fix by using matplotlib/legacycontour which was a fork of the removed private _cntr module. This way we can with minimal changes still support recent and current matplotlib versions.