Skip to content

Fix Cython compilation warnings in NL Means and Watershed#2947

Merged
jni merged 2 commits intoscikit-image:masterfrom
oleksandr-pavlyk:cython-warnings
Mar 8, 2018
Merged

Fix Cython compilation warnings in NL Means and Watershed#2947
jni merged 2 commits intoscikit-image:masterfrom
oleksandr-pavlyk:cython-warnings

Conversation

@oleksandr-pavlyk
Copy link
Copy Markdown
Contributor

Addressed Cython warnings emitted during cythonization step of scikit-image build process:

[1/1] Cythonizing ./work/scikit-image/skimage/restoration/_nl_means_denoising.pyx
warning: skimage/restoration/_nl_means_denoising.pyx:15:26: Declarations should not be declared inline.

[1/1] Cythonizing ./work/scikit-image/skimage/morphology/_watershed.pyx
warning: skimage/morphology/_watershed.pyx:166:24: Index should be typed for more efficient access

I am using Cython 0.27.3.

Per Cython warning, declare type of the index variable to improve
performance and resolve the warning.
As per Cython warning message:

```
[1/1] Cythonizing ./work/scikit-image/skimage/restoration/_nl_means_denoising.pyx
warning: skimage/restoration/_nl_means_denoising.pyx:15:26: Declarations should not be declared inline.
```

cdef extern from "fast_exp.h":
inline double fast_exp(double y) nogil
double fast_exp(double y) nogil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change bring an overhead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so, since it is declared inline in the header fast_exp.h. The warning indicated this need not be duplicated, as best I understood.

@soupault soupault added 🔧 type: Maintenance Refactoring and maintenance of internals 🧐 Needs review labels Jan 25, 2018
@soupault soupault added this to the 0.14.1 milestone Jan 25, 2018
@soupault soupault requested a review from jni March 7, 2018 12:24
@jni
Copy link
Copy Markdown
Member

jni commented Mar 8, 2018

Cool! I pulled the branch and found ~5% speed increase for 2D images and ~15% for 3D images! (The different speed makes sense because pixels in 3D images have more neighbors.) Thanks @oleksandr-pavlyk!

@jni jni changed the title Cython warnings Fix Cython compilation warnings in NL Means and Watershed Mar 8, 2018
@jni jni merged commit b862868 into scikit-image:master Mar 8, 2018
@jni
Copy link
Copy Markdown
Member

jni commented Mar 8, 2018

(My statements above apply to watershed — I don't expect a speedup in NL Means.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧐 Needs review 🔧 type: Maintenance Refactoring and maintenance of internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants