Closed dmsurti closed 8 years ago
The test fails as Travis uses VTK 5.8 which does not have the `ImageSincInterpolator'.
Aside from the test failure, LGTM
@jwiggins Thanks for reviewing. But I had misunderstood the requirement (might be :8ball: effect), which was to change the gaussian
function in gaussian_function_component.GaussianOpacityNode
.
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 :sweat_smile:
Just noticed that you pushed those changes to this PR... Other than the orphaned constants, LGTM!
@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?
Yeah... Window
is pretty good. Or maybe Block
?
PS: Your patriotically-numbered private issue is leaking into this public repo :wink:
:innocent: Nothing to see here...
Block
has the advantage of being less overloaded than Window
. @brendonhall, what do you think?
Both Block
and Window
could work. Semantically, a windows lets you see things that are behind it, whereas blocking something does the opposite.
Just go with Window
before this discussion goes full :bike: shed.
Lots of renaming... LGTM.
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.
No objections here - I agree that the localized behaviour (having the window go to zero) makes more sense.
:+1:
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.
This provides the hamming windowed sinc interpolation method for image data
TBD