bastula / dicompyler

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

Please do not use private module matplotlib._cntr #137

Open tillea opened 3 years ago

tillea commented 3 years ago

Hi, the Debian Med package is maintaining the Debian package of dicompyler. Debian is currently in freeze and we received a bug report of a user and we need to fix this quickly if dicompyler should become part of the next release. The problem is not as the user assumed that matplotlib is missing but dicompyler is not compatible with recent matplotlib / Python3 version. The issue is in dicompyler/baseplugins/2dview.py which contains

from matplotlib import _cntr as cntr

which is illegal. Do you see any chance to fix this in some dedicated patch? Thanks a lot, Andreas.

bastula commented 3 years ago

Hi Andreas,

Hope you are doing well. It is possible to use the legacycontour package available here: https://github.com/matplotlib/legacycontour and follow the instructions by Alan listed here: https://github.com/bastula/dicompyler/issues/122. That should at least allow the program to function correctly with matplotlib >= 2.2.

Another option that is possible is to swap to measure_contours from scikit-image as outlined here: https://github.com/bastula/dicompyler/issues/122#issuecomment-570842862 but introduces a much heavier dependency. One of the goals of dicompyler is to be lightweight.

tillea commented 3 years ago

Hi Aditya,

On Fri, Apr 09, 2021 at 08:18:15PM -0700, Aditya Panchal wrote:

Hope you are doing well. It is possible to use the legacycontour package available here: https://github.com/matplotlib/legacycontour and follow the instructions by Alan listed here: https://github.com/bastula/dicompyler/issues/122. That should at least allow the program to function correctly with matplotlib >= 2.2.

This will not the problem for the Debian package. The module legacycontour is not (yet) packaged for Debian and since we are in freeze currently no new packages are permitted.

Another option that is possible is to swap to measure_contours from scikit-image as outlined here: https://github.com/bastula/dicompyler/issues/122#issuecomment-570842862 but introduces a much heavier dependency. One of the goals of dicompyler is to be lightweight.

I admit I have no idea about the goals and features of dicompyler. I'm not using it and simply took over the maintenance in Debian from some developer who left the team to salvage it for the users of the Debian package. So if you can provide a working patch I will include it in the Debian package which will safe it for the next stable release - otherwise it will be removed from there which would be a shame.

Kind regards, Andreas.

tillea commented 3 years ago

On Sat, Apr 10, 2021 at 07:20:03AM +0200, Andreas Tille wrote:

... which will safe it for the next stable release - otherwise it will be removed from there which would be a shame. This has happened now. It would be great if you could fix this in a next dicompyler release to re-include it in some future Debian release again. Kind regards, Andreas.

jmartens commented 2 years ago

I have proposed a fix in PR #142 for this referencing the original issue #122.

tillea commented 2 years ago

Am Mon, Nov 01, 2021 at 11:49:03PM -0700 schrieb Jonathan Martens:

I have proposed a fix in PR #142 for this referencing the original issue #122. From a Debian packaging point of view this does not help much since legacycontour is not packaged. While I could undergo the packaging legacycontour I would love to avoid packaging something with "legacy" in the name and would be really happy if you would consider the "swap to measure_contours from scikit-image" solution you proposed above. Kind regards, Andreas.

jmartens commented 2 years ago

On 2-11-2021 10:34, Andreas Tille wrote:

From a Debian packaging point of view this does not help much since legacycontour is not packaged. While I could undergo the packaging legacycontour I would love to avoid packaging something with "legacy" in the name and would be really happy if you would consider the "swap to measure_contours from scikit-image" solution you proposed above.

This is a much more invasive fix. My short term goal with this fix is to not rely indefinitely on outdated packages and not use a private function as is currently done.

jmartens commented 2 years ago

@tillea I am trying to understand the need for a package since this is just plain Python code requiring a few modules which could be easily installed using pip install -r requirements. Perhaps you can elaborate on the need for a (Debian) package?

tillea commented 2 years ago

Am Tue, Nov 02, 2021 at 03:30:30AM -0700 schrieb Jonathan Martens:

This is a much more invasive fix. My short term goal with this fix is to not rely indefinitely on outdated packages and not use a private function as is currently done. Can you do some estimation when you might find a solution that can live without legacycontour? Kind regards, Andreas.

tillea commented 2 years ago

Am Tue, Nov 02, 2021 at 03:53:30AM -0700 schrieb Jonathan Martens:

@tillea I am trying to understand the need for a package since this is just plain Python code requiring a few modules which could be easily installed using pip install -r requirements. Perhaps you can elaborate on the need for a (Debian) package? Since all preconditions for a Debian package need to be available as a Debian package. A "simple" pip install is a no-go. Assume you install a number of Debian packages and want to fire up the software which will not work out of the box and you are forced to download a couple of other software, some from pip and some from other sources and you need to do some research what software needs what external code. Kind regards, Andreas.

jmartens commented 2 years ago

Can you do some estimation when you might find a solution that can live without legacycontour? Kind regards, Andreas.

I can't and won't, as I am not dedicated to maintaining this. For me, it is done as I sometimes use dicompyler and the intention of my pull request is to get it running for me with (reasonably) acceptable packages and no errors. From the issues and state of the PRs here it seems that maintenance of the project is not a high priority for others like @bastula as well.