Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a test case to itkPhaseAnalysisSoftThresholdImageFilterTest #38

Open
jhlegarreta opened this issue May 31, 2017 · 5 comments
Open
Labels

Comments

@jhlegarreta
Copy link
Member

The itk::PhaseAnalysisSoftThresholdImageFilterTest has a boolean member variable (with m_ApplySoftThreshold), which would be nice to test when setting to false.

Pablo, I can do it if you wish once PR #34 gets hopefully merged.

phcerdan added a commit that referenced this issue Jun 2, 2017
Change for a warning.

It seems I get different results than Jon in my machine.

```
Expected: 10044.5, but got: 10055.1
Test failed!
Error in GetSigmaAmp()
Expected: 5020.3, but got: 5018.47
Test failed!
Error in GetThreshold()
Expected: 20085.1, but got: 20092
```

See Issue #38
@phcerdan
Copy link
Collaborator

phcerdan commented Jun 2, 2017

Thanks! I had to remove the hard check between doubles: GetAmp(), GetThreshold, etc.
My machine has different results than yours!
I have put a warning... but maybe the cleanest way would be to increase the tolerance parameter in itkNotAlmostEqual , see phcerdan@9676ac2

@jhlegarreta
Copy link
Member Author

The differences are notable. Looks weird; may be the floating point precision issues get accumulated and end up in relatively large errors in this case? The itkNotAlmostEqual function, when calling the right template function, does count on a default, invariable tolerance value; however, the itk::Math::FloatAlmostEqual does accept an input tolerance parameter, so we'd be able to use it to account for differences accross systems in this case. In fact, it is done frequently in ITK tests. So once an explanation for the difference found, I'd vote for it.

However, I'm intrigued by the difference; may be @thewtex @fbudin69500 can provide more insight on the large differences observed once they get some time?

@thewtex
Copy link
Member

thewtex commented Jun 2, 2017

Yes, those differences are larger that what is usually observed for floating point comparisons, so may be there is some numerical improvements or bugs we could address. The itkPhaseAnalysisSoftThresholdImageFilterTest.cxx also uses the MonogenicSignalFrequencyImageFilter, so maybe we should take a step back and ensure that its output is numerically stable?

Side note or possibly related, can this code:

https://github.com/phcerdan/ITKIsotropicWavelets/blob/9676ac21d2e23ee33597bd1418bc780eec9fccf4/include/itkPhaseAnalysisSoftThresholdImageFilter.hxx#L100-L115

be placed in BeforeThreadedGenerateData and the barrier's removed?

@phcerdan
Copy link
Collaborator

phcerdan commented Jun 3, 2017

@thewtex yeah, good point, better to test the base PhaseAnalysis and the MonogenicSignal. Working with phase stuff tends to be really sensible to float errors, but let's see.

However not sure if moving that barrier code to BeforeThreadedGenerateData is possible. The SoftThreshold filter uses Superclass::ThreadedGenerateData to calculate the amplitude and phase. And then it calculates the mean, and variance of the amplitude image (full image region). I would try to explore the base class first.

@thewtex
Copy link
Member

thewtex commented Jun 4, 2017

Once option to have multiple multi-threaded operations in a filter is to use the itk::DomainThreader. This is like the ImageSource's BeforeThreadedThreadedGenerateData, ThreadedGenerateData, and AfterThreadedGenerateData, but isolated to a class that can be used multiple times throughout a filter.

It currently requires quite a bit of code to set up, but it is an an option.

@phcerdan phcerdan added the test label Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants