C matrices #158
C matrices #158
Conversation
JoostJM
force-pushed
the
c-matrices
branch
3 times, most recently
from
33628e6
to
f06982c
Compare
|
Snapshot from profiling, C functions are highlighted:
|
JoostJM
force-pushed
the
c-matrices
branch
6 times, most recently
from
cec483b
to
005d4aa
Compare
|
Finished implementing time intensive features in C. Calculation time at profile now just over 6 seconds for 5 testcases. Still writing some docstrings.
When GLDZM (EXPERIMENTAL, REMOVED) is merged into the master, I can also write a C function for this. This is very simple, as it would be identical to the one calculating the GLSZM, but instead of counting the processed voxels, it records the minimum distance in the zone. |
|
When I do @JoostJM did you set up a paid account on TravisCI to proceed with CI testing on Mac? @naucoin could help setting it up (see #126) once we have a paid account (or open source version of pyradiomics). |
|
@jcfr, Can you resubmit your changes? I'm afraid I accidentally deleted them when I rebased this branch on the master, to incorporate the changes from the other PR's we just merged. |
|
@fedorov as to your error, did you have numpy and cython installed prior to running this script? |
|
Sure. Will do after lunch. On Nov 23, 2016 1:17 PM, "Joost van Griethuysen" notifications@github.com
|
|
@JoostJM @pieper @fedorov Here it is. Let me know what you think. As a side note, @isuruf is helping to add scikit-build into conda forge. See conda-forge/staged-recipes#1989 |
|
@jcfr My build is failing as I do not have CMake currently installed on my system. So this means your update needs the user to have CMake installed, as well as the extra dependencies. If we're okay with, it's fine with me. |
|
I am not worried about CMake as a dependency. I am worried about maintaining CMakeLists.txt files. But this might well be a necessary price to pay if you decide to use C. |
|
@JoostJM The following should do it. There is also more details in the commit message. |
|
@jcfr No, I already did that, I ran your line again and it just says requirements satisfied |
|
|
|
I recall from somewhere that using MinGW is a bad idea ... welcome to the C world! |
|
Do you have Visual Studio 2008 installed ? This is the official compiler for python 2.7 I have to head out and will look at it later. And also improve the documentation of scikit-build for alternative and how to use them. Example: https://www.microsoft.com/en-us/download/details.aspx?id=44266 ((The issue with building from the Git shell is that there is a mingw compiler also installed ...)) |
|
I have Visual Studio 2015, but my environmentvariables are set so that VS90COMNTOOLS points to this newer compiler, so without using cmake, my python usually just finds my compiler |
or or More details here: http://scikit-build.readthedocs.io/en/latest/usage.html#command-line-options I will look into |
Package is now built into `_skbuild`, update gitignore to ignore this folder.
Fix potential index out of range error in `fill_glszm` in tempData. This occurs when the entire tempData matrix is filled (Ns different regions. When the function fill_glszm then runs, it does not exit the while loop (as it encounters no -1 at the end of the matrix). Specify the size of the tempData matrix = (Ns + 1) * 2, so that the last element will always be -1 (max idx computed by function calculate_glszm is still Ns * 2). Additionally re-apply code style to C files
This approach allows to support use of radiomics package after installing pyradiomics as a regular binary distribution
|
scikit-build has been released. Update of this topic will follow. |
Remove C code for calculating the matrix in these classes. Remove the tests for these classes from test_cmatrices
Commit 7da5b76 caused the dynamic lookup of feature classes in `featureextractor` to break. Fix the function and move this lookup functionality to `__init__.py`, as it is used more extensively throughout the package.
JoostJM
force-pushed
the
c-matrices
branch
2 times, most recently
from
0fc72a0
to
81638df
Compare
This should be re-enabled after integrating #158
|
@jcfr I updated my scikit-build and cmake on my computer at work, but I am still getting errors. It's an unresolved external reference error in the load-function of numpy (only fails when compiling with cmake though). Here is a section the setup output, do you know whats going wrong?
|
This should be re-enabled after integrating #158
|
@JoostJM Note that I started to rebase and re-organized commits. I will follow up with some update. |
|
@JoostJM Here is what I have so far: https://github.com/Radiomics/pyradiomics/tree/c-matrices-rebased-jcfr I squashed together changes that made sense. Still need some work. Would be nice to integrate separately all changes that are not specific to C implementation. For example: f6b77ed |
|
Closing. This is superseded by #200 |
Implement C for calculation of matrices
Currently supports:
This greatly improves processing speed, while still keeping an all-python option available (currently enabled by setting
radiomics.debuggingtoTrue.Also included automatic linking and compiling to setup script. Needs additional package (
cython) to work, but is now also included in setup script (if not installed, setup script installs it without error).Lastly, added a new test, where the python generated matrices are compared to the C generated matrices for all test cases. No features are calculated here, but matrices are assessed element wise (maximum difference for any element is 1e-3, this is due to small machine precision errors possible in NGTDM).