making it python 3 compatible #188

kmader · 2017-02-09T10:52:41Z

without breaking the python 2 compatibility (#178), adding the future imports to keep others clean and behavior particularly division consistent.

The current changes should change or break nothing with python 2 execution and only make the code itself runnable on 3.

…python 2 compatibility, adding the future imports to keep others clean and behavior particularly division consistent

…, adding a new notebook for simple tests, fixing typo in safe_xrange, most tests pass now, but still differences, probably because SimpleITK, numpy, and PyWavelets have changed.

JoostJM · 2017-02-10T10:33:20Z

@kmader Thank you for your work on making PyRadiomics compatible with Python3! I'm currently reviewing your changes. Could you also enable CircleCI testing? I don't see the results. Travis and Appveyor show failing tests, but this is due to the fact that it is not yet set up correctly (see also #183).

Additionally, I'm seeing an error in shape._calculate2DMaxDiameter, which has to be addressed before we can merge.

In python 3, the zip function returns a 'zip' object, similar to python generators. This does not change it's implementation in python for loops, but it is not evaluated if it is used to generate a numpy array. Therefore, add a cast to list, which evaluates the object, before assigning it to the numpy array. Also, use relative imports in tests. Finally, fix some print-function bugs in the 'bin' and 'batchexamples' folder and rerun the python notebooks, so the output reflects the changes made.

JoostJM · 2017-02-10T13:11:23Z

@kmader, In python 3, the implementation of the zip() function is changed, which then returns a zip object that must be cast first to e.g. a list object before using it to create a numpy array. This was not necessary in python 2, and not used in the PyRadiomics code. This broke one function. I pushed a commit which fixes that bug, and reran all notebooks, no errors.

Also, there were some python 2-style print calls left in the bin folder, so I also corrected those.

JoostJM

LGTM, but I'd like to test it using CircleCI before I approve.

fedorov · 2017-02-10T13:57:00Z

Any ideas why CircleCI build is not being triggered for this PR?

I have problems with this PR on mac:

$ nosetests --with-xunit --logging-level=DEBUG --verbosity=3 tests/test_features.py
nose.config: INFO: Ignoring files matching ['^\\.', '^_', '^setup\\.py$']
Failure: ValueError (Attempted relative import in non-package) ... ERROR

======================================================================
ERROR: Failure: ValueError (Attempted relative import in non-package)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
    addr.filename, addr.module)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/Users/fedorov/github/pyradiomics/tests/test_features.py", line 7, in <module>
    from .testUtils import RadiomicsTestUtils, custom_name_func
ValueError: Attempted relative import in non-package
-------------------- >> begin captured logging << --------------------
pykwalify.compat: DEBUG: Using yaml library: /Users/fedorov/anaconda/lib/python2.7/site-packages/yaml/__init__.pyc
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
XML: /Users/fedorov/github/pyradiomics/nosetests.xml
----------------------------------------------------------------------
Ran 1 test in 0.005s

FAILED (errors=1)

The current master works fine.

fedorov · 2017-02-10T14:01:47Z

@jcfr FYI I disabled Appveyor build when no appveyor.yml is present in the org settings

Also did the same for TravisCI

kmader · 2017-02-10T14:14:24Z

@JoostJM thanks, that zip problem I had just ignored because it worked mostly fine without it (I calculate shape features on my own). Why did you change
this

from testUtils import RadiomicsTestUtils, custom_name_func

To this

from .testUtils import RadiomicsTestUtils, custom_name_func

The python3 style AFAIK is to just use the top way

fedorov · 2017-02-10T14:23:15Z

@kmader if I undo the change by @JoostJM I get the following errors on mac:

$ nosetests --with-xunit --logging-level=DEBUG --verbosity=3 tests/test_features.py
nose.config: INFO: Ignoring files matching ['^\\.', '^_', '^setup\\.py$']
Failure: TypeError (__name__ must be set to a string object) ... ERROR

======================================================================
ERROR: Failure: TypeError (__name__ must be set to a string object)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
    addr.filename, addr.module)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/Users/fedorov/github/pyradiomics/tests/test_features.py", line 21, in <module>
    class TestFeatures:
  File "/Users/fedorov/github/pyradiomics/tests/test_features.py", line 38, in TestFeatures
    @parameterized.expand(generate_scenarios(), testcase_func_name=custom_name_func)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose_parameterized-0.5.0-py2.7.egg/nose_parameterized/parameterized.py", line 356, in parameterized_expand_wrapper
    frame_locals[name] = cls.param_as_standalone_func(p, f, name)
  File "/Users/fedorov/anaconda/lib/python2.7/site-packages/nose_parameterized-0.5.0-py2.7.egg/nose_parameterized/parameterized.py", line 366, in param_as_standalone_func
    standalone_func.__name__ = name
TypeError: __name__ must be set to a string object
-------------------- >> begin captured logging << --------------------
pykwalify.compat: DEBUG: Using yaml library: /Users/fedorov/anaconda/lib/python2.7/site-packages/yaml/__init__.pyc
testUtils: DEBUG: RadiomicsTestUtils
testUtils: DEBUG: Reading baseline for class firstorder
testUtils: DEBUG: Reading baseline for class glcm
testUtils: DEBUG: Reading baseline for class glrlm
testUtils: DEBUG: Reading baseline for class glszm
testUtils: DEBUG: Reading baseline for class shape
root: DEBUG: generate_scenarios: featureNames = ['10Percentile', '90Percentile', 'Energy', 'Entropy', 'InterquartileRange', 'Kurtosis', 'Maximum', 'MeanAbsoluteDeviation', 'Mean', 'Median', 'Minimum', 'Range', 'RobustMeanAbsoluteDeviation', 'RootMeanSquared', 'Skewness', 'StandardDeviation', 'TotalEnergy', 'Uniformity', 'Variance']
testUtils: DEBUG: custom_name_func: function name = test_scenario, param_num = 000, param.args = ('brain2', 'firstorder', '10Percentile')
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
XML: /Users/fedorov/github/pyradiomics/nosetests.xml
----------------------------------------------------------------------
Ran 1 test in 0.006s

FAILED (errors=1)

kmader · 2017-02-10T14:35:34Z

@fedorov maybe try adding absolute_import from __future__ import print_function, unicode_literals, division, absolute_import
or if that fails the less elegant

try:
    from .testUtils import RadiomicsTestUtils, custom_name_func
except ImportError:
    from testUtils import RadiomicsTestUtils, custom_name_func

fedorov · 2017-02-10T14:45:56Z

This does not help :-(

fedorov · 2017-02-10T14:50:50Z

@kmader can you confirm you tested this with Python 2?

JoostJM · 2017-02-13T08:41:29Z

@kmader, @fedorov, I used the relative import as my IDE wasn't recognizing the modules. However, if you want to use relative imports in python 2.7, it must be a package (i.e. has an __init__.py file in the folder), as I didn't add this (error), it did not work. Additionally, in PR #158 most parts of PyRadiomics use relative imports within the package (and is the testing folder also marked as 'package'). @jcfr, did you have a specific reason for implementing this?
As to the errors, I now reverted back to an absolute import. If we want to change this in the future, this can be done in PR #158 or in a separate PR.

JoostJM · 2017-02-13T10:06:21Z

@fedorov, @kmader, I found the 2nd error. The problem was that due to the compatibility changes, the custom_name_func returned a unicode string instead of a string. This then breaks the parameterized.expand function of nosetesting. Fixed it using a forced cast to str()

JoostJM · 2017-02-13T12:23:25Z

@fedorov, @kmader, I have nose running locally for both python 2.7 and 3.5, but we haven't caught all issues yet. Currently I am working on a bug (filter function does not work properly in python 3.5 and as such deletes several diagonals in glrlm, causing large enough errors to fail the tests). Moreover, some other functions, mainly in glcm are also differing slightly from python 2.7 generated baseline, but these errors are fairly small.

Some functions expect string types (such as in nose-testing), but python 3.5 by default provides unicode strings. Add forced casts where necessary. Also, 'wb' mode requires strings to be encoded as bytes, not string in python 3.5. The fix did not work in this case. Change mode to 'w', which accepts regular strings. Additionally, safe_cmp produced errors for dictionary comparison in python 3.5, replace by `==` comparison. Finally, update layout in testing files (such as replace `!=` by `is not` in comparisons to `None`). Update feature name in testing output to also reflect the feature class.

In python 3.5 GLRLM results differed from baseline. This due to the different implementation of the `filter()` function, which returns an iterator instead of a list. In GLRLM calculation, there where 2 iterations over this iter-object (one for flat-angle check, one for matrix calculation), causing it to break in python 3.5. Fix by merging the 2 iterations into one loop. Additionally, simplify the filter function.

JoostJM · 2017-02-13T13:59:33Z

Any ideas why CircleCI build is not being triggered for this PR?

This is due to the fact that this PR is based out of @kmader's repository, and if it doesn't have a webhook defined for CircleCI, no tests are run. I ran CircleCI tests for both python 2.7 and python 3.5 locally (Windows) and received no errors after my last commit.

If I look at the diff-file generated, there are some functions with diff != 0, but the difference is always in the order of e-16, so most likely computer precision or epsilon difference...

As far as I can see, this PR is now ready for merging, with PyRadiomics running both on python 2.7 and python 3.5

fedorov · 2017-02-13T14:25:19Z

It works for me as well now.

About CircleCI - I still don't understand why it doesn't work. Webhooks should only affect builds of the branches for @kmader repository, but I thought incoming PRs for pyradiomics should always be built, and rely on pyradiomics webhooks setup.

JoostJM · 2017-02-13T14:29:36Z

About CircleCI - I still don't understand why it doesn't work. Webhooks should only affect builds of the branches for @kmader repository, but I thought incoming PRs for pyradiomics should always be built, and rely on pyradiomics webhooks setup.

Not in it's current form, but let me take a look at the webhooks of this repository

JoostJM · 2017-02-13T14:38:11Z

@fedorov, I found the problem, the webhook is configured to also send a notification on PRs from forked repos, but in CircleCI, the building of PRs from forked repos was turned off. It should work now.

JoostJM · 2017-02-13T14:39:09Z

Redelivered webhook after last commit. CircleCI now testing...

fedorov · 2017-02-13T14:45:32Z

Makes sense now, thanks!

# Conflicts: # radiomics/__init__.py # radiomics/featureextractor.py # tests/test_docstrings.py

JoostJM · 2017-02-13T15:51:45Z

Merged the new master to resolve conflicts. Rebase impossible, as this requires a force push (not allowed on master branch).

fedorov · 2017-02-13T15:53:46Z

this requires a force push (not allowed on master branch)

I was wondering why this PR comes from the master branch. How do you guys plan to sync it with pyradiomics master?

JoostJM · 2017-02-13T15:56:26Z

I was wondering why this PR comes from the master branch. How do you guys plan to sync it with pyradiomics master?

Just like any other PR, as this is 4Quant:master branch, not pyradiomics:master. However, for future PRs, it is still advisable to do so on a non-master branch, as thing such as rebasing are easier.

fedorov · 2017-02-13T16:00:12Z

I suggest you disable force-push on 4Quant master, force 4Quant master to be identical to the current master, make a branch for this PR, and make a new PR off that branch.

I would recommend against merging the commit history as it shows up at the moment for this PR.

JoostJM · 2017-02-14T09:11:37Z

@kmader, I will rebase this branch onto the new master, but I can't push it to your repository, so I'll push it to the central repo and create a new PR.

kmader · 2017-02-14T09:35:09Z

@JoostJM that sounds fine, thanks!

JoostJM · 2017-02-14T09:41:40Z

superseded by #194

kmader added 2 commits February 9, 2017 11:51

making it python 3 compatible (AIM-Harvard#178) without breaking the …

72ac3cb

…python 2 compatibility, adding the future imports to keep others clean and behavior particularly division consistent

fixing all side classes to be python 3 compatible, updating notebooks…

5b1e8bb

…, adding a new notebook for simple tests, fixing typo in safe_xrange, most tests pass now, but still differences, probably because SimpleITK, numpy, and PyWavelets have changed.

JoostJM reviewed Feb 10, 2017

View reviewed changes

BUG: Revert to using absolute import within tests modules

72649ec

JoostJM added 2 commits February 13, 2017 14:48

Merge remote-tracking branch 'upstream/master'

c480d28

# Conflicts: # radiomics/__init__.py # radiomics/featureextractor.py # tests/test_docstrings.py

jcfr mentioned this pull request Feb 13, 2017

ci: Generalize continuous integration adding support for appveyor and travis #183

Merged

JoostJM mentioned this pull request Feb 14, 2017

making it python 3 compatible_REVISED #194

Closed

JoostJM closed this Feb 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

making it python 3 compatible #188

making it python 3 compatible #188

kmader commented Feb 9, 2017 •

edited

Loading

JoostJM commented Feb 10, 2017

JoostJM commented Feb 10, 2017

JoostJM left a comment

fedorov commented Feb 10, 2017

fedorov commented Feb 10, 2017 •

edited

Loading

kmader commented Feb 10, 2017

fedorov commented Feb 10, 2017

kmader commented Feb 10, 2017

fedorov commented Feb 10, 2017

fedorov commented Feb 10, 2017

JoostJM commented Feb 13, 2017 •

edited

Loading

JoostJM commented Feb 13, 2017 •

edited

Loading

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017 •

edited

Loading

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 14, 2017 •

edited

Loading

kmader commented Feb 14, 2017

JoostJM commented Feb 14, 2017

making it python 3 compatible #188

making it python 3 compatible #188

Conversation

kmader commented Feb 9, 2017 • edited Loading

JoostJM commented Feb 10, 2017

JoostJM commented Feb 10, 2017

JoostJM left a comment

Choose a reason for hiding this comment

fedorov commented Feb 10, 2017

fedorov commented Feb 10, 2017 • edited Loading

kmader commented Feb 10, 2017

fedorov commented Feb 10, 2017

kmader commented Feb 10, 2017

fedorov commented Feb 10, 2017

fedorov commented Feb 10, 2017

JoostJM commented Feb 13, 2017 • edited Loading

JoostJM commented Feb 13, 2017 • edited Loading

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017

JoostJM commented Feb 13, 2017 • edited Loading

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 13, 2017

fedorov commented Feb 13, 2017

JoostJM commented Feb 14, 2017 • edited Loading

kmader commented Feb 14, 2017

JoostJM commented Feb 14, 2017

kmader commented Feb 9, 2017 •

edited

Loading

fedorov commented Feb 10, 2017 •

edited

Loading

JoostJM commented Feb 13, 2017 •

edited

Loading

JoostJM commented Feb 13, 2017 •

edited

Loading

JoostJM commented Feb 13, 2017 •

edited

Loading

JoostJM commented Feb 14, 2017 •

edited

Loading