move the location of fast_exp to help compilation with strange flags

[hmaarrfk]

Description

So I rewrote fast_exp as a cython function
#3866

not sure if this is any better or any worse than the previous impelmentation (in terms of speed). This is faster by a good factor of 5. Could be really important as mentioned below for large datasets.

Personally, I think it is some kind of micro-optimization, but I can see where this might be actually helpful consider the restricted range of a uint8 image

Will need to add a benchmark, not too thrilled about doing that at the moment, but I wanted to give somebody a patch to get this working on their "unsupported" architecture.

Checklist

• Docstrings for all functions
• Gallery example in ./doc/examples (new features only)
• Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests
• Clean style in the spirit of PEP8

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.
• Consider backporting the PR with @meeseeksdev backport to v0.14.x

[stefanv]

Mark, thanks for doing this work. My initial thought is the same as yours: should we be implementing such low level behavior in scikit-image? Can we use something from Python or Cython or NumPy or SciPy instead?

[hmaarrfk]

The counter argument there is that often innovation comes from those who need it.
numpy is mostly interested in "double precision accuracy" (as things are naturally up-cast to doubles). Image data is more focused on "small numbers from 0-255" and as such, shortcuts like this might be worthwhile, if the assumptions are justified.

often innovation comes from those who need it. I don't want to inhibit innovation. But I also thing that "speedups" should be quantified and understand that they are very compiler dependent.

I don't think it is necessarily wrong to implement such things like this here. I think maybe we can move it to the one file that needs it. I'm getting headaches from pyd and pyx.....

The primary goal of this PR is to ensure we are compatible with as many "compiler options" as possible, as demonstrated by a user running Alpine linux which seems to disable some optimization that would otherwise have respected the inline suggestion.

[hmaarrfk]

Ok, the approximation is pretty cool:

I think we should keep it, maybe just move it into the file that needs it to simplify the programming model.

[hmaarrfk]

Looking into things differently, it seems the error can be as high as 4% in certain cases. That isn't too encouraging.

[hmaarrfk]

@stefanv see the commit message
57c042d#diff-917fcc21c44771a2336c8559bacbfa74

Further more the discussion here
#2878

States that we should probably keep the fast_exp function.

I think folding fast_exp into the nl_means pyx is probably the best way to ensure correct compilation while respecting the author's contributions.

I'll work on that later maybe.

[hmaarrfk]

@blochl i totally didn't see your great PR a while back.

I think one the scikit-image policies has been not to include the author names in specific files but rather in the CONTRIBUTORS.txt file. I added a little blurb about your contributation.

Mostly I moved this inside the nl_means so that it would work with some strange compilation options.

Need to test this on an alpine linux build. But I mostly wanted to let you know that your work wasn't really getting erased.

[jni]

I thought NL-means was implemented by @emmanuelle? And git blame corroborates my memory...

[grlee77]

Yes, Emma contributed the non-local means function and myself and others have made some improvements over the years.

I would describe the contribution here as "Improvements to the performance and stability of non-local means restoration."

[jni]

@hmaarrfk @stefanv I'd favour including this, since NL-means is an intrinsically slow technique, so a speedup of this magnitude can have a major impact. Having said this, the contributor notes are now incorrect (afaict, correct me if I'm wrong), so that needs fixing.

[jni]

@hmaarrfk would you be able to add a benchmark for NLmeans and report on the results from this PR?

[stefanv]

Apart from the attribution, this looks fine. Another, possibly simpler, solution, would be to leave the .h file alone, and to add a .pxd that defines the inline function. Either way, it is fine with me.

[jni]

@hmaarrfk I tried to rebase it, but I got confused about whether these changes actually got partly merged somewhere else, or how they would interact with np_floats. Would you like to rescue this PR or should we close it?

[jni]

Ping @hmaarrfk

I tried to rebase it, but I got confused about whether these changes actually got partly merged somewhere else, or how they would interact with np_floats. Would you like to rescue this PR or should we close it?

[grlee77]

closing as likely resolved by the changes in #4322. Please reopen if this is not the case.