[WIP]: Use hamming window function

[dmsurti]

This provides the hamming windowed sinc interpolation method for image data

TBD

• Refactor

[dmsurti]

The test fails as Travis uses VTK 5.8 which does not have the `ImageSincInterpolator'.

[jwiggins]

Aside from the test failure, LGTM

[dmsurti]

@jwiggins Thanks for reviewing. But I had misunderstood the requirement (might be 🎱 effect), which was to change the gaussian function in gaussian_function_component.GaussianOpacityNode.

[jwiggins]

Oh... Yes, that's something else entirely. I'm happy to review that change when it comes. I'm the guilty party that wrote it 😅

[jwiggins]

Looks like MIN_GAUSSIAN_STD and GAUSSIAN_RADIUS_STD_SCALE can be removed now, no?

[jwiggins]

Just noticed that you pushed those changes to this PR... Other than the orphaned constants, LGTM!

[noraderam]

It would be nice to have a smaller minimum radius. If the radius is too small I get weird behavior. Making this 0.005 seems to work fine

[noraderam]

@dmsurti As you mentioned in the discussion elsewhere, the naming needs an update. Gaussian is obviously no longer applicable. Because it's so easy to swap out different window functions, I think it would be better to use a name that isn't specific to the implementation. That would also lay the groundwork for different types of windows in the future.

One piece of feedback that we got was that Add Gaussian... was not enough information for the user; when the other options are Add Color... and Add Opacity, it's not clear that the Gaussian adds both color and opacity. In light of that, perhaps we could name the action Add Color/Opacity Window and rename everything that calls itself Gaussian -> Window

@jwiggins, any opinions about the naming?

[jwiggins]

Yeah... Window is pretty good. Or maybe Block?

PS: Your patriotically-numbered private issue is leaking into this public repo 😉

[noraderam]

😇 Nothing to see here...

[noraderam]

Block has the advantage of being less overloaded than Window. @brendonhall, what do you think?

[brendonhall]

Both Block and Window could work. Semantically, a windows lets you see things that are behind it, whereas blocking something does the opposite.

[jwiggins]

Just go with Window before this discussion goes full 🚲 shed.

[jwiggins]

Lots of renaming... LGTM.

[noraderam]

All of the changes look good. I did notice one strange thing when testing out the new window with the examples/ctf_demo.py script.

On master, the Gaussian window ends at 0, with the rest of the transfer function flat to the endpoints

It looks like the Hamming window ends at a higher value, giving an unexpected curve to the resulting transfer function.

I imagine that this is inherent to the choice of window. Any objections to switching to a hanning window, instead? That was the other initially proposed option and it does not exhibit this behavior.

[brendonhall]

No objections here - I agree that the localized behaviour (having the window go to zero) makes more sense.

[noraderam]

Hamming -> Hanning

[noraderam]

A couple other comments need updating as well

[noraderam]

👍

A bit more work will be required to accomplish the requested task, but it's good to get the renaming work done. I expect that we'll be updating the window used again within the next couple days.