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

Frangi Vesselness Filter #525

Merged
merged 18 commits into from
Dec 5, 2017
Merged

Frangi Vesselness Filter #525

merged 18 commits into from
Dec 5, 2017

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Oct 5, 2017

This PR adds an implementation for Frangi's Multiscale Vessel Enhancement Filter, a filter that is given a grayscale, white-on-black image of vessel structures and returns an image that highlights the vessel structures and removes noise.

This PR closes issue #490.

Copy link
Member

@stelfrich stelfrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the implementation. Thanks for your work @gselzer! I'd suggest to reuse the imglib2 implementation Intervals.numElements() instead of an own implementation of DefaultFrangi.numberOfPoints(). Otherwise it's just minor stuff...

* allowing users to specify:
* 1) The scales desired for the filter application
* 2) The physical spacing between image data points
* 3) Whether or not the pre-Gaussian filter should be performed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the term "pre-Gaussian" a bit misleading. To me, this implies that a Gaussian is applied to an image before the computation. In practice, however, it is applied for each scale, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I fixed it.

@Parameter
private Img<T> input;

@Parameter(label = "Gauss before:")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like "Apply Gaussian at each scale:" would be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds better.

private boolean doGauss;

@Parameter(label = "Spacing",
description = "Physical distance between the observed data points")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't "Physical distance" imply something like 3 microns? From the implementation, I'd say that is more of a "relative distance": 1,1 implies that we have the same spacing in x and y, whereas 1,1,2 implies that the resolution in z is half of the resolution in x and y.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is a better way of expressing that.


@Override
public void run() {
//parse the spacing, and scales strings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intendation is wrong. Plus, you have used a space in comments before (which is also the preferred way according to the coding style).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@@ -47,6 +47,7 @@
import net.imglib2.algorithm.neighborhood.Shape;
import net.imglib2.img.Img;
import net.imglib2.outofbounds.OutOfBoundsFactory;
import net.imglib2.type.BooleanType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did that come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone now!

frangi(input, output, scale);
}

private void frangi(RandomAccessibleInterval<T> in, RandomAccessibleInterval<U> out, int step) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess we could add that.

@@ -104,6 +104,7 @@ namespaces = ```
[name: "paddingIntervalOrigin", iface: "PaddingIntervalOrigin"],
[name: "sigma", iface: "Sigma", aliases: ["sigmaFilter", "filterSigma"]],
[name: "variance", iface: "Variance", aliases: ["varianceFilter", "filterVariance", "var", "varFilter", "filterVar"]],
[name: "vesselness", iface: "Vesselness"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frangiVesselness?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this makes sense to do.


cursor.fwd();


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

double bd = 2 * beta * beta;

// OutOfBoundsMirrorStrategy for use when the cursor reaches the edges.
OutOfBoundsMirrorFactory<T, RandomAccessibleInterval<T>> osmf = new OutOfBoundsMirrorFactory<T, RandomAccessibleInterval<T>>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make this a @Parameter of the op and set the OutOfBoundsMirrorFactory as default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the benefit of this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, @gselzer!

We have made the OutOfBoundsFactory a @Parameter in other implementations, so you can feed in another one if you like. I understand that it is an additional parameter to provide, that in practice most user will not overwrite. So I am fine with leaving it as it is, for the sake of usability. However, I wanted to state that this is one of the pitfalls of ImageJ1: making implicit
assumptions about the out-of-bounds strategy without being able to change it...

Maybe @ctrueden has an opinion on that topic as well?

return new Context(OpService.class, OpMatchingService.class, CacheService.class, ScriptService.class);
}

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed in the clean up commit.

@@ -104,7 +104,7 @@ namespaces = ```
[name: "paddingIntervalOrigin", iface: "PaddingIntervalOrigin"],
[name: "sigma", iface: "Sigma", aliases: ["sigmaFilter", "filterSigma"]],
[name: "variance", iface: "Variance", aliases: ["varianceFilter", "filterVariance", "var", "varFilter", "filterVar"]],
[name: "vesselness", iface: "Vesselness"],
[name: "frangiVesselness", iface: "FrangiVesselness"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with the word refactoring—it means to change the internal structure of code without changing its external behavior. So no API changes! In this case, the API definitely does change because the op interface name changed from Vesselness to FrangiVesselness. So this is not a refactoring. A better commit message would be "Rename vesselness to frangiVesselness."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@@ -104,6 +104,7 @@ namespaces = ```
[name: "paddingIntervalOrigin", iface: "PaddingIntervalOrigin"],
[name: "sigma", iface: "Sigma", aliases: ["sigmaFilter", "filterSigma"]],
[name: "variance", iface: "Variance", aliases: ["varianceFilter", "filterVariance", "var", "varFilter", "filterVar"]],
[name: "frangiVesselness", iface: "FrangiVesselness"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now go to f in the alphabet (and indentation fixed), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. You are right. I will change it here and in the namespace.

@ctrueden
Copy link
Member

ctrueden commented Dec 5, 2017

Rebased over latest master. All tests passing, so merging now.

@ctrueden ctrueden merged commit edd4513 into master Dec 5, 2017
@ctrueden ctrueden deleted the issue490 branch December 5, 2017 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants