Some AnalyzeSkeleton detailed branch results are not calibrated

[mdoube]

Describe the bug
Detailed branch information reported by BoneJ's AnalyzeSkeleton and Skeleton option in Particle Analyser is uncalibrated

To Reproduce
Steps to reproduce the behavior:

1. Run AnalyzeSkeleton on a skeletonised image that has spatial calibration, selecting detailed branch information
2. Run Analyse Skeleton from BoneJ's menus on the same image
3. Note that the Euclidean Distance and branch location values are different

Expected behavior
Output from BoneJ's calls to AnalyzeSkeleton's methods should exactly match output from AnalyzeSkeleton

Additional context
Reported here:
https://forum.image.sc/t/skeleton-branch-length-producing-zero-values-for-most-of-the-soil-pores-in-bonej2/38505/6

[rimadoma]

I hope the issue is in how we call AnalyzeSkeleton_, and not in what comes into AnalyseSkeletonWrapper. That is, debugging should start by checking that calibration is OK in the wrapper's ImagePlus Parameter

[mdoube]

It seems like 'branch length' is calibrated but the junction locations and Euclidean distance is not. This may be an inconsistency in the API that we could fix upstream, or else we forgot to apply the calibration downstream.

[mdoube]

we forgot to apply the calibration downstream

We have a winner. This is the relevant section of the parent plugin:
https://github.com/fiji/AnalyzeSkeleton/blob/c6baa1f95c68037b1cc98358957585d19488316a/src/main/java/sc/fiji/analyzeSkeleton/AnalyzeSkeleton_.java#L1461

extra_rt.addValue(extra_head[3], e.getV1().getPoints().get(0).x * this.imRef.getCalibration().pixelWidth);
extra_rt.addValue(extra_head[4], e.getV1().getPoints().get(0).y * this.imRef.getCalibration().pixelHeight);
extra_rt.addValue(extra_head[5], e.getV1().getPoints().get(0).z * this.imRef.getCalibration().pixelDepth);

Points here are integer pixel coordinates and are being multiplied by image calibration to get real unit locations. Calibration is always applied very late.

[mdoube]

@rimadoma @alessandrofelder wondering why we wrap the skeleton plugins at all? Not sure there is a particular value in simply repeating the third-party plugin. Only advantage seems to be in cases where there is a single skeleton that the results can be aggregated in the BoneJ result table. Skeletonise is maybe needlessly duplicated? Shouldn't Maven and the updater take care of dependency issues?

[alessandrofelder]

I guess backwards compatibility (scripts that people use that call the BoneJ version, and users that are used to clicking through to the BoneJ version) is another one. But I agree that neither seems like a particularly strong argument.

Needs to be weighed up against the time we spend maintaining the plugin?

[mdoube]

I ask because deleting duplicated code is a good way to fix the bugs in it.

[mdoube]

We musn't be scared of breaking people's scripts when we've made such a big move from BoneJ1 to 2. Their scripts will end up being more robust. and not a big fix: switching correctly spelled Analyse to wrongly spelled Analyze is all.

(Analyze with a z, I mean, what were they thinking??? One can argue the toss about Skeletonize vs Skeletonise, but Analyze is just wrong.)

[alessandrofelder]

The implementation of ITA uses the unwrapped analyzeSkeleton, so no problems there.
Not duplicating code is a good argument, so for me, we can remove it.

To be clean and consistent with our semantic versioning, we should move to BoneJ2 8.0.0 due to an API change?

[rimadoma]

wondering why we wrap the skeleton plugins at all?

Some reasons in no particular order:

• There were minor differences in the functionality of the BoneJ1 plugins compared to the originals, which are now reflected in the wrappers. E.g. Skeletonize3D_ overwrites its input images, while SkeletoniseWrapper doesn't
• Clearer UX: e.g. SkeletoniseWrapper cancels with a message if image is not binary, Skeletonize3D_ happily processes the Blobs sample image, which while 8-bit greyscale is not binary
• Bundling (so that BoneJ > Skeletonise etc are there)
• Potential support for hyper stacks (generally I know how to do this, but there are snags in the required conversions)
• Potentially providing an API into the legacy plugins for ImgPlus (again conversion problems I haven't figured out)
• Consistency in Results (as you mentioned)
• Consistency in Python etc scripting (@alessandrofelder How does writing Python on top of legacy plugins work)?
• Other assumptions that have since flown out of the window

I'm not saying any of these are a reason not to remove the wrappers. Just things we need to consider if we decide to do so.

[rimadoma]

If we get rid of the wrappers, we could offer our stricter image validation downstream to @iarganda

[alessandrofelder]

Re Python scripting: I don't think it will be a large fix for script users.

I think you can call Legacy and other ImageJ1.x stuff via

import IJ

IJ.run("Some Command", ...) 

and Modern plugins (also?) via

# @CommandService cs

wrapper = cs.run("org.bonej.wrapperPlugins.EllipsoidFactorWrapper",False,["inputImgPlus",source,"nVectors", vectors, ...])
wrapperInstance = wrapper.get()
outputs = wrapperInstance.getOutput("ellipsoidFactorOutputImages");

I guess if there are many people calling BoneJ's version the Modern way from a script, and the underlying skeletonisation API is Legacy, it's a slightly larger fix. As Michael says, we've changed the API a lot recently anyway, so probably not too bad? 🤷