whitematteranalysis icon indicating copy to clipboard operation
whitematteranalysis copied to clipboard

Add Python3 support

Open jcfr opened this issue 5 years ago • 6 comments

These changes are expected to be compatible with Python 2 and can be integrated.

Note that more work is required to fully support Python 3

jcfr avatar Apr 17 '19 05:04 jcfr

Thank you JC this is amazing.

ljod avatar Apr 17 '19 21:04 ljod

@jcfr thanks for doing this. Looks like many of the changes in ee7abcb and the change in e3238a9 have been merged through other PRs, e.g. #118.

A few notes:

  • Python 2 reached EOL a while a go, so maintaining Python 2 compatibility does not look like it is worthwhile now.
  • Some commits (e.g. 2ea70c4, ed13549) seem not to be in line with modern, Python 3 practices.
  • Using a dot is still a relative import (i.e. 2ea70c43644f68280a0be27b9cc398dc6d705f56), right? Slightly related, going forward, we should import the modules needed, and not the whole whitematternaalysis.

Do you want to rebase this on master or drop some commits and see what remains to be improved or do you think the PR should be closed?

jhlegarreta avatar Sep 21 '23 23:09 jhlegarreta

Notes after investigating further the changes:

  • BUG: Fix syntax of Slicer4/TractCluster/test.py e3238a9 : not integrated. The module is gone. Needs investigation why.
  • STYLE: Update python scripts to use print function for python 3.x ee7abcb :
    • It is unfortunate that tests and testing data were removed in commit 7c71fb1. We need to recover those (at least all parts that still apply).
    • Print statements were modernized in commit da3d285, PR #90.
  • STYLE: Update python classes to follow new-style ed13549 : not integrated. Subclassing object is unnecessary since Python 3.x. Can drop this commit.
  • STYLE: Update use of except and raise to work with Python 3.x 561748a : the modified two files are gone. Needs investigation why 7c71fb1 removed them.
  • STYLE: Update python scripts to use absolute import for python 3.x 2ea70c4 : wma relative (because these are relative imports, not absolute) imports were adopted in da3d285, PR #90. Can drop this commit.

jhlegarreta avatar Oct 06 '23 20:10 jhlegarreta

Thanks for working on integrating.

jcfr avatar Oct 06 '23 22:10 jcfr

Do not hesitate to rebase the pull-request, I will not have to bandwidth to work on this.

jcfr avatar Oct 06 '23 22:10 jcfr

Thanks @jcfr. Will work on this as time permits (long-term effort). Thanks for your past contributions to the tool.

jhlegarreta avatar Oct 06 '23 22:10 jhlegarreta