FrequencyBandImageFilter accepts lambdas for filtering frequencies #149

dzenanz · 2018-11-07T21:01:19Z

This filter should enable custom filtering via the functor, e.g. implementing Butterworth algorithm.

blowekamp · 2018-11-07T21:14:04Z

Should FrequencyBandImageFilter be derived from the UnaryGeneratorFilter?

phcerdan

This is great!!! Functors working with frequencies... the dream.

I wonder if we could refactor all the functor love into a base class.
And the old behavior (the boxFunctor), just out in its own header as a functor struct (a class with operator() and the options RadialBand etc, as members)

Modules/Filtering/ImageFrequency/test/itkFrequencyBandImageFilterTest.cxx

dzenanz · 2018-11-07T21:23:02Z

@blowekamp maybe it should. I was busy trying to get the functionality, It didn't even occur to me that I could derive it from UnaryGenerator. But since the functor should take the FrequencyIterator instead of just the pixel value, I am afraid deriving from UnaryGenerator might be more trouble than it is worth.

Also, UnaryFunctor filter existed when FrequencyBandImageFilter was originally written. It must have made some sense not to derive it from UnaryFunctor.

phcerdan · 2018-11-07T21:24:53Z

The great lambdas/functor work of @blowekamp in UnaryGenerator was posterior to the development of this filter.

dzenanz · 2018-11-07T21:29:11Z

@phcerdan Do not confuse UnaryGenerator filter which is pretty new, and UnaryFunctor filter which is very old.

dzenanz · 2018-11-07T21:31:38Z

@phcerdan If backwards compatibility is not a major issue, we could refactor the filter as you propose. What do others think? @thewtex @hjmjohnson @blowekamp

blowekamp · 2018-11-07T21:37:17Z

+1 for separating the FrequencyBandImageFilter into a "Frequency Generator Filter" base class and a "Box Functor". They could come together into the current class similar to the "AddImageFilter" combines the BinaryGeneratorImageFilter and the AddFunctor.

phcerdan · 2018-11-07T21:37:28Z

I think the only module using it (heaviliy) is ITKIsotropicWavelets, and I can fix that when needed. Also, to avoid breakage of backwards compatibility, we can create the functor, and then create a FrequencyBandImageFilter just plugging together the base class and the new boxFunctor.

phcerdan · 2018-11-07T21:38:26Z

It seems I agree with @blowekamp 😄 (submitted the comment almost at the same time)

thewtex · 2018-11-07T21:40:35Z

I don't think the frequency classes have been in a final release, so there should not be backwards compatibility concerns. 🆕

dzenanz · 2018-11-07T21:45:10Z

Great! Let me verify that this works as intended, then I can polish it per suggestions.

phcerdan · 2018-11-07T21:46:16Z

By the way, BandPassFunctor should be a better name.

dzenanz · 2018-11-07T21:48:55Z

A better name for what is now known as BoxFunctor, or for what is now known as FrequencyBandImageFilter?

Edit: Butterworth is also a BandPassFunctor.

phcerdan · 2018-11-07T21:51:21Z

A better name for BoxFunctor.

And maybe we can change the name from FrequencyBandImageFilter to FrequencyBandPassImageFilter. I like the name with ...Pass but could be misleading because it works as well as a BandBlock depending on options...

hjmjohnson · 2018-11-07T23:35:31Z

Thanks all. This is good work.

blowekamp · 2018-11-08T14:48:31Z

With the new GitHub workflow hopefully it is easier to manage topic branches. Please separate the nice documentation improvement in itkUnaryGeneratorFilter into separate commit to help keep a clear history.

dzenanz · 2018-11-08T17:32:45Z

This is working as expected. I will refactor this patch to separate the box functor into its own class and add additional Butterworth functor.

dzenanz · 2018-11-08T21:05:38Z

In the end I did not add Butterworth filter here, but as its implementation is short it can be defined in appropriately adjusted form by the user.

blowekamp

This looks really good!

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.hxx

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.h

phcerdan

BoxFunctor should be changed to the more descriptive BandPassFunctor. Box is not describing what the functor is doing.

Modules/Filtering/ImageFrequency/include/itkFrequencyBandImageFilter.hxx

dzenanz · 2018-11-09T20:39:04Z

I think this is ready for merging. Can you re-review please?

phcerdan

This is great, thanks!!

blowekamp · 2018-11-09T21:07:32Z

Looks good!

Does this class of filters support streaming? If not then EnlargeOutputRequestedRegion should be overloaded to make the output the largest possible.

dzenanz · 2018-11-09T21:21:15Z

@blowekamp why should the OutputRequestedRegion be enlarged?

blowekamp · 2018-11-09T21:31:38Z

The default behavior of the ITK pipeline (e.g. ImageToImageFilter) is to have the requested region of the output be copied to the inputs. This enables streaming for basic filters like the unary functors etc.. This filter operates with frequency space, I believe an output pixel needs more information than just the corresponding input pixel.

One way to disable streaming is to overload EnlargeOutputRequestedRegion to set the produced region to always be the largest possible region. By default the output region gets copied to the input. This is done when the filter only works on the whole image.

Other options depend on if and how the filter can stream.

dzenanz · 2018-11-09T21:40:35Z

This filter is templated over the frequency iterator. The iterator uses the meta-information to know the frequency of the currently iterated pixel. And a reference to this iterator is passed to the functor, so it can access the frequency etc. Which means that this filter should probably be able to stream. I haven't tested that, though.

blowekamp · 2018-11-09T21:48:02Z

This filter is templated over the frequency iterator. The iterator uses the meta-information to know the frequency of the currently iterated pixel. And a reference to this iterator is passed to the functor, so it can access the frequency etc. Which means that this filter should probably be able to stream. I haven't tested that, though.

If the filter only has part of the input, then the boundary conditions would likely cause difference frequencies to be processes by the iterator.

It may be that the filter can produce an arbitrary output but needs the full input. In that case the GenerateInputRequestedRegion should be overloaded to set the input's requested region to the largest possible.

dzenanz · 2018-11-09T21:51:32Z

This is essentially a per-pixel filter. The only complication is that the iterator "knows" the frequency/index of the pixel, which is used as a criteria for modifying the value. I don't think the input's region needs to be expanded.

blowekamp · 2018-11-09T21:56:00Z

Ok, I got how the iterator operate now. Thanks!

phcerdan · 2018-11-09T22:08:34Z

For me, it helps to think that the input and output images are already in the frequency domain. The only role of the frequency iterators is to avoid the change of the algorithms depending on the Fourier transform or frequency layout.
So the developer thinks directly in frequencies, instead of what indices correspond to what frequencies.

thewtex

Some warnings identified by the macOS build:

https://open.cdash.org/viewBuildError.php?type=1&buildid=5622223

dzenanz · 2018-11-10T13:41:05Z

@thewtex I fixed that with the latest push. All checks are now passing.

thewtex

Very nice @dzenanz !

Some minor issues noted inline.

Modules/Filtering/ImageFrequency/include/itkFrequencyBandImageFilter.hxx

thewtex · 2018-11-10T17:02:06Z

Modules/Filtering/ImageFrequency/include/itkFrequencyBandImageFilter.hxx

+    }
+  else // Cut-off box taking into account max absolute frequency.
+    {
+    w = freqIt.GetFrequency();


wector frequency? uv? @phcerdan might give the answer 😄

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.h

thewtex · 2018-11-10T17:10:28Z

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.h

+ * This filter is templated over a TFrequencyIterator depending on the
+ * frequency layout of the input image.
+ * Images in the dual space can be acquired experimentally, from scattering exaperiments or other techniques.
+ * The layout of these images is the same as spatial domain images. Use \ref FrequencyImageRegionIteratorWithIndex


What is the meaning of this sentence? Can it be reworded to clarify that meaning?

I reworded it. @phcerdan could double-check.

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.h

Changes applied.

UnaryFrequencyDomainFilter accepts lambdas for filtering frequencies. Functor setting is modeled after UnaryGeneratorImageFilter.

phcerdan · 2018-11-13T15:20:45Z

Sorry, I was unplugged on the weekend, sorry for not answering on time. But I would have approved, scalarFrequency is a way better name than the old w. Thanks @dzenanz

dzenanz requested review from thewtex and phcerdan November 7, 2018 21:01

dzenanz mentioned this pull request Nov 7, 2018

Band-pass frequencies InsightSoftwareConsortium/ITKMontage#43

Closed

thewtex requested a review from samuelgerber November 7, 2018 21:17

phcerdan approved these changes Nov 7, 2018

View reviewed changes

Modules/Filtering/ImageFrequency/test/itkFrequencyBandImageFilterTest.cxx Outdated Show resolved Hide resolved

Modules/Filtering/ImageFrequency/test/itkFrequencyBandImageFilterTest.cxx Outdated Show resolved Hide resolved

dzenanz force-pushed the master branch from a9f5199 to 21ce303 Compare November 7, 2018 21:29

dzenanz force-pushed the master branch from 21ce303 to 1cbea49 Compare November 8, 2018 21:03

blowekamp reviewed Nov 8, 2018

View reviewed changes

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.hxx Outdated Show resolved Hide resolved

Modules/Filtering/ImageFrequency/include/itkUnaryFrequencyDomainFilter.h Show resolved Hide resolved

phcerdan requested changes Nov 8, 2018

View reviewed changes

Modules/Filtering/ImageFrequency/include/itkFrequencyBandImageFilter.hxx Outdated Show resolved Hide resolved

dzenanz force-pushed the master branch from 8d624e3 to 1a487b1 Compare November 9, 2018 20:35

dzenanz changed the title ~~WIP: FrequencyBandImageFilter accepts lambdas for filtering frequencies~~ FrequencyBandImageFilter accepts lambdas for filtering frequencies Nov 9, 2018

phcerdan approved these changes Nov 9, 2018

View reviewed changes

thewtex requested changes Nov 9, 2018

View reviewed changes

dzenanz force-pushed the master branch from 1a487b1 to 74cf69a Compare November 9, 2018 23:55

thewtex previously requested changes Nov 10, 2018

View reviewed changes

DOC: fixing copy-paste error about functor type

57bb11d

dzenanz force-pushed the master branch from 74cf69a to 77d7efc Compare November 10, 2018 17:41

thewtex approved these changes Nov 12, 2018

View reviewed changes

ENH: FrequencyBandImageFilter derives from UnaryFrequencyDomainFilter

29980f3

UnaryFrequencyDomainFilter accepts lambdas for filtering frequencies. Functor setting is modeled after UnaryGeneratorImageFilter.

dzenanz force-pushed the master branch from 77d7efc to 29980f3 Compare November 12, 2018 21:06

dzenanz merged commit fc23208 into InsightSoftwareConsortium:master Nov 13, 2018

FrequencyBandImageFilter accepts lambdas for filtering frequencies #149

FrequencyBandImageFilter accepts lambdas for filtering frequencies #149

Conversation

dzenanz commented Nov 7, 2018 • edited Loading

blowekamp commented Nov 7, 2018

phcerdan left a comment

Choose a reason for hiding this comment

dzenanz commented Nov 7, 2018

phcerdan commented Nov 7, 2018 • edited Loading

dzenanz commented Nov 7, 2018

dzenanz commented Nov 7, 2018

blowekamp commented Nov 7, 2018

phcerdan commented Nov 7, 2018

phcerdan commented Nov 7, 2018 • edited Loading

thewtex commented Nov 7, 2018

dzenanz commented Nov 7, 2018

phcerdan commented Nov 7, 2018

dzenanz commented Nov 7, 2018 • edited Loading

phcerdan commented Nov 7, 2018 • edited Loading

hjmjohnson commented Nov 7, 2018

blowekamp commented Nov 8, 2018

dzenanz commented Nov 8, 2018

dzenanz commented Nov 8, 2018

blowekamp left a comment

Choose a reason for hiding this comment

phcerdan left a comment

Choose a reason for hiding this comment

dzenanz commented Nov 9, 2018

phcerdan left a comment

Choose a reason for hiding this comment

blowekamp commented Nov 9, 2018

dzenanz commented Nov 9, 2018

blowekamp commented Nov 9, 2018

dzenanz commented Nov 9, 2018

blowekamp commented Nov 9, 2018

dzenanz commented Nov 9, 2018

blowekamp commented Nov 9, 2018

phcerdan commented Nov 9, 2018

thewtex left a comment

Choose a reason for hiding this comment

dzenanz commented Nov 10, 2018

thewtex left a comment

Choose a reason for hiding this comment

thewtex Nov 10, 2018

Choose a reason for hiding this comment

dzenanz Nov 10, 2018

Choose a reason for hiding this comment

thewtex Nov 10, 2018

Choose a reason for hiding this comment

dzenanz Nov 10, 2018

Choose a reason for hiding this comment

thewtex Nov 10, 2018

Choose a reason for hiding this comment

phcerdan commented Nov 13, 2018

dzenanz commented Nov 7, 2018 •

edited

Loading

phcerdan commented Nov 7, 2018 •

edited

Loading

phcerdan commented Nov 7, 2018 •

edited

Loading

dzenanz commented Nov 7, 2018 •

edited

Loading

phcerdan commented Nov 7, 2018 •

edited

Loading