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

Feature/diffusion add an-isotropic diffusion to vmtkimagesmoothing #249

Merged
merged 6 commits into from Mar 13, 2018

Conversation

Projects
None yet
2 participants
@kayarre
Contributor

kayarre commented Mar 2, 2018

Adding additional smoothing filter to vmtkimagesmoothing using ITK an-isotropic diffusion.

The anisotropic diffusion feature includes the ability to work with 2D or 3D and on many variable types.

@rlizzo rlizzo self-assigned this Mar 2, 2018

@rlizzo

This comment has been minimized.

Member

rlizzo commented Mar 2, 2018

This is great Kurt! awesome job. I'm just digging into this now, so expect updates soon.

Just one issue off the bat with the branch history. it seems that this was based off of your prior PR #196, which we decided to close and integrate the changes directly into vmtksurfaceendclipper in PR #200. Basically, this means that the current history of the PR would recreate the previously proposed changes and create a new vmtkScripts/vmtksurfaceclippercenterline.py.

I'm going to attempt to fix this now so that the proposed changes are based off the current master branch, will let you know if I run into any weird issues that would better be handled by you as author of the PR.

@rlizzo

This comment has been minimized.

Member

rlizzo commented Mar 2, 2018

Ok. So the history is kindof a mess and can't be easily fixed by me via a single merge or few cherry picks (atleast while preserving history with you as the author). I think it'll be easiest for you to handle on your end.

Following the git log here, we see that 822a3e9 is based off of f45a8fd and 70f4349, which is in turn based off of 5aaaa16 (which was current state of master roughly 5 months ago). As f45a8fd and 70f4349 were part of our rejected PR, this leaves the history significantly out of date.

image

You need to pull the latest upstream master branch into your fork's master. Then checkout your feature/diffusion branch and rebase it off of master. This fixed it in my tests. It's probably cleanest to then submit the rebased branch as a seperate PR so that the public histories align.

@kayarre

This comment has been minimized.

Contributor

kayarre commented Mar 2, 2018

@kayarre

This comment has been minimized.

Contributor

kayarre commented Mar 5, 2018

@rlizzo I am sorry about this headache I created. I was able to rebase/revert create a new commit and force it to feature/diffusion. I tried to follow your instructions while also trying to fix my remote master.

}
// using PixelType = float;

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Remove unused code, or state why it is temporarily commented out

break;
}
case VTK_CHAR:

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Shouldn't a char type fail every time. Not sure that the processing algorithm would be able to handle chars

This comment has been minimized.

@kayarre

kayarre Mar 5, 2018

Contributor

This portion was from something similar in ITK. I happy to remove the char type. I would assume that in most cases that makes sense. I don't have a good idea of which types are or should be handled.

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Ok. Yeah, from the documentation (https://itk.org/Doxygen/html/classitk_1_1GradientAnisotropicDiffusionImageFilter.html) it seems that only scalar image types are supported. Would probably be best to just remove it to prevent confusion in the future.

This comment has been minimized.

@kayarre

kayarre Mar 6, 2018

Contributor

What's the definition of a scalar type?
I tried this in slicer where I cast an image to char and filtered with anisotropic diffusion and it worked without a problem. I believe there is a separate way to check whether the input image is a scalar image volume which i think addresses the scalar type issue. otherwise there is no reason the filter can't operate on each type?

is the concern here that we need to test each data type? or something else?
in all likelihood it's not an often used use case, but I have a hard time not being thorough.

This comment has been minimized.

@rlizzo

rlizzo Mar 13, 2018

Member

I was a bit confused it seems. I've been doing a bit of Java and got confused between the C++ chars and Java chars (which are essentially unicode characters). The VTK_CHAR is just an 8 bit value (integers between 0 and 255), which seems like it's fine for this application.

My understanding is that a scalar type (used in this context) is any numeric value that VTK can handle. In any case, it's not an issue, sorry about the confusion!

self.StandardDeviation = 1.0
self.RadiusFactor = 5.0
self.Dimensionality = 3
self.Conductance = 1.0
self.NumberOfIterations = 5
self.TimeStep = 0.0625

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Not sure if this timestep is the best default. For the standard image we use in our test cases, I recieved the following output while using the default parameters.

...
Automatic piping vmtkimagesmoothing
    Image = vmtkimagereader-0.Image
Parsing options vmtkimagesmoothing
    Method = anisotropic
Explicit piping vmtkimagesmoothing
Input vmtkimagesmoothing members:
    Id = 0
    Disabled = 0
    Image = vtkImageData
    ImageInputFileName =
    Method = anisotropic
    StandardDeviation = 1.0
    RadiusFactor = 5.0
    Dimensionality = 3
    Conductance = 1.0
    NumberOfIterations = 5
    TimeStep = 0.0625
    ImageOutputFileName =
Executing vmtkimagesmoothing ...
WARNING: In /Users/rick/anaconda3/envs/vmtkbuild/include/ITK-4.13/itkAnisotropicDiffusionImageFilter.hxx, line 82
GradientAnisotropicDiffusionImageFilter (0x7fc46cf20600): Anisotropic diffusion unstable time step: 0.0625
Stable time step for this image must be smaller than 0.0549316

WARNING: In /Users/rick/anaconda3/envs/vmtkbuild/include/ITK-4.13/itkAnisotropicDiffusionImageFilter.hxx, line 82
GradientAnisotropicDiffusionImageFilter (0x7fc46cf20600): Anisotropic diffusion unstable time step: 0.0625
Stable time step for this image must be smaller than 0.0549316

WARNING: In /Users/rick/anaconda3/envs/vmtkbuild/include/ITK-4.13/itkAnisotropicDiffusionImageFilter.hxx, line 82
GradientAnisotropicDiffusionImageFilter (0x7fc46cf20600): Anisotropic diffusion unstable time step: 0.0625
Stable time step for this image must be smaller than 0.0549316

WARNING: In /Users/rick/anaconda3/envs/vmtkbuild/include/ITK-4.13/itkAnisotropicDiffusionImageFilter.hxx, line 82
GradientAnisotropicDiffusionImageFilter (0x7fc46cf20600): Anisotropic diffusion unstable time step: 0.0625
Stable time step for this image must be smaller than 0.0549316

WARNING: In /Users/rick/anaconda3/envs/vmtkbuild/include/ITK-4.13/itkAnisotropicDiffusionImageFilter.hxx, line 82
GradientAnisotropicDiffusionImageFilter (0x7fc46cf20600): Anisotropic diffusion unstable time step: 0.0625
Stable time step for this image must be smaller than 0.0549316

Done executing vmtkimagesmoothing.
Output vmtkimagesmoothing members:
    Id = 0
    Image = vtkImageData
...

This comment has been minimized.

@kayarre

kayarre Mar 5, 2018

Contributor

The time step comes from (PixelSpacing)/2^{N+1}, so for 1.0/2^{4}=0.0625
This is the default in slicer, which I have used (obviously this warning message is hidden). I was thrown off at first, but there is really no way to know apriori what the user wants. We could have the script check for the largest dimension and calculate the minimum time step as the default and only override is the user specifies one?
the challenge here is that obfuscates the interface a little because the script will print out the default time step so another message would be required to inform the user that the timestep is now different.

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Ok. thanks for the clarification!

Since I'm assuming that we want to use the correct value (calculated from the equation) for most people, in most cases, what if we included a method to auto-calculate-timestep, along with an option which defaulted to use that? We could then return the used timestep as an output member to the script, ensuring that the user could backtrack and figure out which value was used (sort of like how isosurface value can be returned in http://www.vmtk.org/vmtkscripts/vmtkimageinitialization.html).

If the auto-calculate-timestep option was enabled, it would just overwrite whatever value of self.TimeStep was provided in the __init__ method. If it was disabled, we would use the self.TimeStep value provided by default, or override it automatically with a user defined value if they decided they wanted to tweak it manually.

I think that would give us both customizability, and standardized logging in one go?

This comment has been minimized.

@kayarre

kayarre Mar 6, 2018

Contributor

fantastic idea, will implement this. I also am going to multiply the calculated time step by a factor close to but less than 1 so it doesn't throw the warning.

@rlizzo

This comment has been minimized.

Member

rlizzo commented Mar 5, 2018

Hey kurt, thanks for fixing this up. The history seems to be in order, and this is a great addition! really nice work!

I added a couple comments in the review above, but in general things seem to work well. After we discuss the details above, we just need to create a few tests for the new parameters. I wrote up a short bit recently on how to contribute tests. You can find it here. Let me know if you have any questions or issues with how the testing process should work, would love to get your feedback on how to improve the documentation.

Thanks again! let me know your thoughts!

Module: $RCSfile: vtkvmtkAnisotropicDiffusionImageFilter.cxx,v $
Language: C++
Date: $Date: 2006/04/06 16:48:25 $
Version: $Revision: 1.1 $

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Update Date to current and Version to 1.4

Date: $Date: 2006/04/06 16:48:25 $
Version: $Revision: 1.1 $
Copyright (c) Luca Antiga, David Steinman. All rights reserved.

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Copyright (c) Luca Antiga. Remove David Steinman

This software is distributed WITHOUT ANY WARRANTY; without even
the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE. See the above copyright notices for more information.

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Add note:

  Note: this class was contributed by 
	Kurt Sansom
	(affiliation or github username if desired)
Module: $RCSfile: vtkvmtkAnisotropicDiffusionImageFilter.h,v $
Language: C++
Date: $Date: 2006/04/06 16:48:25 $
Version: $Revision: 1.2 $

This comment has been minimized.

@rlizzo

rlizzo Mar 5, 2018

Member

Update date and version number to 1.4.

add commented note:

  Note: this class was contributed by 
       Kurt Sansom
        (affiliation or github username if desired)
@kayarre

This comment has been minimized.

Contributor

kayarre commented Mar 5, 2018

@rlizzo thank you for the input. I will take a look at the testing later this week.

kayarre and others added some commits Mar 6, 2018

@rlizzo

This comment has been minimized.

Member

rlizzo commented Mar 13, 2018

I made a few changes and added the test cases. Merging this now! Thanks for the contribution @kayarre !

@rlizzo rlizzo merged commit b4f2f61 into vmtk:master Mar 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment