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 Op discovery annotations to ImgLib2-algorithm #94

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Feb 25, 2022

This PR uses therapi-runtime-javadoc to declare all static functionality in imglib2-alrogithm as Ops. See this test for how these Ops can be called, and the module imglib/imglib2-algorithm-optest for an example of the structure required to call these annotated methods as Ops.

A new dependency on therapi-runtime-javadoc-scribe is added to the maven compiler plugin. For now, this was added to the imglib2-algorithm POM. Once pom-scijava-base 15.0.0 is propagated to imglib2-algorithm, we can remove almost everything added to this POM by this PR.

The only line needed once we depend on that version of pom-scijava-base is this line, which is responsible for informing therapi-runtime-javadoc-scribe of the set of packages it should process. This line must be changed once we bump the version, as the property used is under a different name.

Using therapi-runtime-javadoc-scribe, JSON files are generated for each file containing javadoc in each package specified by therapi.packages

Open questions:

  • Op naming: I have not yet come up with a simple answer to this. I thought it would be nice to name the Ops using the method name, but some methods across different classes have the same name. We could name them using a camel-case "class.method_name", which might match well with how these methods would be called from a static context...
  • Which packages should be passed to therapi? We could just pass all packages, but this would retain javadoc that would not be used. We could also only pass packages with Op declarations, but it would be easy to forget to add new packages to this property when ops are declared in new packages. I tend to like the latter a little better, but don't feel too strongly.
  • What to do with static methods not conforming to an Op? Many static methods are essentially org.scijava.function.Computers with their output parameter in a position that is not the last one; for these, I have added another method with a different parameter ordering. Other methods do not conform to Op types, such as the HessianMatrix methods. We could easily write a new FunctionalInterface for these guys, but they wouldn't have the support of OpBuilder, adaptation, etc.

TODO:

  • The build seems to fail in Java 8, but it runs just fine in Java 11. I still need to figure out why that is... 🤔

@gselzer
Copy link
Contributor Author

gselzer commented Feb 25, 2022

A running list of static methods that do not conform to functional interfaces in scijava-function:

@gselzer gselzer marked this pull request as ready for review March 4, 2022 17:45
@gselzer gselzer force-pushed the ops-annotations branch 4 times, most recently from 4811d07 to 1765d54 Compare December 21, 2023 22:47
@gselzer gselzer marked this pull request as draft December 21, 2023 23:56
@gselzer
Copy link
Contributor Author

gselzer commented Dec 21, 2023

@ctrueden @tpietzsch I've fully updated this PR such that it now uses (the unreleased) SciJava Ops Indexer. I can now automatically discover all annotated static methods in an environment containing both a local build of imglib2-algorithm and a local build of the SciJava Incubator. The PR will not build until we release the SciJava Ops Indexer, so I've left it as a draft, but bringing this work up-to-date made me excited for the possibilities of SciJava Ops!

@hinerm
Copy link
Member

hinerm commented Jul 2, 2024

@tpietzsch In this PR we are discussing the minimum Java version to index libraries with Ops. I'm wondering if there is a need to keep imglib2-algorithm at Java 8, or if going to Java 11 is an option?

@tpietzsch
Copy link
Member

@hinerm I don't think there is anything that would break with higher Java versions. As soon as Fiji finally moves to Java 21, (all of) ImgLib2 will too.

@gselzer
Copy link
Contributor Author

gselzer commented Aug 26, 2024

TODO:

There are some methods whose signatures do not conform to the Ops style (e.g. here), where I've written a wrapper method that has been opified. The question, then, is what to do with the original methods. Should we consider deprecating them, @ctrueden?

@gselzer gselzer force-pushed the ops-annotations branch 12 times, most recently from 678df37 to 66cd368 Compare August 26, 2024 23:29
@gselzer gselzer reopened this Aug 26, 2024
Copy link
Contributor Author

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

@ctrueden @elevans I'd appreciate your opinions here!

@@ -105,7 +105,8 @@ public final class MserTree< T extends Type< T > > implements ComponentForest< M
* {@link #buildMserTree(RandomAccessibleInterval, RealType, long, long, double, double, ImgFactory, boolean)}
* using an {@link ArrayImgFactory} or {@link CellImgFactory} depending on
* input image size.
*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinions on whether these should be Ops? I'm not totally sure what they're used for...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

And I guess this article is relevant.

Copy link
Member

Choose a reason for hiding this comment

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

While we are figuring this stuff out, we should improve the javadoc. Every algorithm should cite the paper(s) it is about in its class javadoc. A "component tree" in the context of the componenttree packages is explained in papers like this one; without that context, it is difficult to understand what all this code is for.

Copy link
Member

Choose a reason for hiding this comment

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

Concepts of level sets, mathematical morphology, and connected component analysis are very related. We have CCA currently in labeling, but other morphology operators in morphology. What about level sets? FWIW, I checked scikit-image for comparison, and use of level sets mostly takes place in skimage.segmentation.morphsnakes.

@@ -74,6 +74,7 @@ public final class PixelListComponentTree< T extends Type< T > > implements Comp
* using an {@link ArrayImgFactory} or {@link CellImgFactory} depending on
* input image size.
*
* @implNote op name='create.pixelListComponentTree',type=Function
* @param input
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like above, should these be ops?

Copy link
Member

Choose a reason for hiding this comment

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

Can we use a sub-namespace of create here? create.componentTree.pixelList, create.componentTree.mser, and so on? It would match the package naming. In general, looking at the Java package naming is a good starting point for naming suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Actually... are these really "create" Ops? I think of create as merely allocating and returning an object instance, not doing any computation around an image. So this PixelListComponentTree shouldn't be a create Op at all, since it accepts an existing image as input. morphology.componentTree.pixelList maybe? I don't know this space of image algorithms well enough to feel qualified to suggest anything. We could use @tpietzsch's input here, since he wrote these classes. And/or just read the papers about these things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually seems to be like the tree best belongs to the features namespace, as it is my understanding that component trees themselves have distilled down a 2D function into a tree, where the position is lost. This seems more like feature extraction than morphology.

src/main/java/net/imglib2/algorithm/util/Grids.java Outdated Show resolved Hide resolved
@@ -56,6 +56,8 @@ public class Kernel1D
* @param halfKernel
* the upper half (starting at the center pixel) of the symmetric
* convolution kernel.
* @return a {@link Kernel1D} used for one dimensional convolutions
* @implNote op name='create.kernel1DSymmetric'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should these be Ops? What is the right name?

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to group all the kernel creation ops into create.kernel. sub-namespace? E.g. create.kernel.symmetric1D? Or maybe create.kernel1D.symmetric since these all return Kernel1D objects. Not sure...

@gselzer gselzer marked this pull request as ready for review August 26, 2024 23:37
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

OK, I expressed some opinions. Hope it helps.

@@ -122,6 +122,7 @@ public static enum DISTANCE_TYPE
* should be squared, too).
* @param <T>
* {@link RealType} input
* @implNote op name='image.distanceTransform', type=Inplace1
Copy link
Member

Choose a reason for hiding this comment

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

This can be morphology.distanceTransform.

@@ -105,7 +105,8 @@ public final class MserTree< T extends Type< T > > implements ComponentForest< M
* {@link #buildMserTree(RandomAccessibleInterval, RealType, long, long, double, double, ImgFactory, boolean)}
* using an {@link ArrayImgFactory} or {@link CellImgFactory} depending on
* input image size.
*
Copy link
Member

Choose a reason for hiding this comment

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

And I guess this article is relevant.

@@ -105,7 +105,8 @@ public final class MserTree< T extends Type< T > > implements ComponentForest< M
* {@link #buildMserTree(RandomAccessibleInterval, RealType, long, long, double, double, ImgFactory, boolean)}
* using an {@link ArrayImgFactory} or {@link CellImgFactory} depending on
* input image size.
*
Copy link
Member

Choose a reason for hiding this comment

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

While we are figuring this stuff out, we should improve the javadoc. Every algorithm should cite the paper(s) it is about in its class javadoc. A "component tree" in the context of the componenttree packages is explained in papers like this one; without that context, it is difficult to understand what all this code is for.

@@ -105,7 +105,8 @@ public final class MserTree< T extends Type< T > > implements ComponentForest< M
* {@link #buildMserTree(RandomAccessibleInterval, RealType, long, long, double, double, ImgFactory, boolean)}
* using an {@link ArrayImgFactory} or {@link CellImgFactory} depending on
* input image size.
*
Copy link
Member

Choose a reason for hiding this comment

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

Concepts of level sets, mathematical morphology, and connected component analysis are very related. We have CCA currently in labeling, but other morphology operators in morphology. What about level sets? FWIW, I checked scikit-image for comparison, and use of level sets mostly takes place in skimage.segmentation.morphsnakes.

@@ -78,7 +78,8 @@ public class TopHat
* decomposition. Each shape is processed in order as given in the list. If
* the list is empty, the source image is returned.
* <p>
*
*
* @implNote op names='morphology.topHat, morphology.whiteTopHat', type=Function
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop the morphology.topHat alias, in favor of whiteTopHat always, for symmetry with blackTopHat. That's what scikit-image does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, SciJava Ops Image currently only uses morphology.topHat. I agree that symmetry would be good, but I'm also tempted to leave in the aliases. Maybe we can deprecate that soon?

src/main/java/net/imglib2/algorithm/util/Grids.java Outdated Show resolved Hide resolved
These are TBD and are not currently used by anyone, so we can re-opify
them later!
Should probably be all lowercase
These Ops are not used, and there is ambiguity about how they might be
used so let's punt on the decision to opify
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.

Declare imglib2 functions as ops
4 participants