-
Notifications
You must be signed in to change notification settings - Fork 24
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
Also show measured distances in vx (in addition to nm) #5240
Changes from 5 commits
ffd7678
e917865
453f866
45fd2bd
fb4feaa
a6c97c9
1a64420
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -633,7 +633,10 @@ class TracingApi { | |
return result; | ||
} | ||
|
||
measureTreeLength(treeId: number): number { | ||
/** | ||
* Measures the length of the given tree and returns the length in nanometer and in voxels. | ||
*/ | ||
measureTreeLength(treeId: number): [number, number] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes to the api's interface are a bit unfortunate. The probability that someone uses this feature is probably very low and the effort to fix it is quite small. This is why I don't want to bump the api version (which is quite a bit of overhead for us and also for our users which have to check how to upgrade). This is a good example why our current API versioning is sub-ideal :/ Do you agree with this approach, @MichaelBuessemeyer and @daniel-wer? Alternatively, I could keep the old methods and add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a third variant:
You can do the same steps for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevertheless, I think your solution is fine. You can go ahead and merge this PR if you are against my proposed variant. Users that would have to adapt their code should be able to handle this small interface change 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that would also work (and is kind of what I meant with my suggestion to have an additional method à la There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing that this method has only existed for a couple of months and its addition wasn't announced, I would also think usage is probably extremely small if not zero. You could include this change in the "breaking" section of the changelog :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good idea! I also agree with your risk assessment so this PR should be good to go now 🕺 |
||
const skeletonTracing = assertSkeleton(Store.getState().tracing); | ||
const tree = skeletonTracing.trees[treeId]; | ||
if (!tree) { | ||
|
@@ -644,28 +647,39 @@ class TracingApi { | |
|
||
// Pre-allocate vectors | ||
|
||
let lengthAcc = 0; | ||
let lengthNmAcc = 0; | ||
let lengthVxAcc = 0; | ||
for (const edge of tree.edges.all()) { | ||
const sourceNode = tree.nodes.get(edge.source); | ||
const targetNode = tree.nodes.get(edge.target); | ||
lengthAcc += V3.scaledDist(sourceNode.position, targetNode.position, datasetScale); | ||
lengthNmAcc += V3.scaledDist(sourceNode.position, targetNode.position, datasetScale); | ||
lengthVxAcc += V3.length(V3.sub(sourceNode.position, targetNode.position)); | ||
} | ||
|
||
return lengthAcc; | ||
return [lengthNmAcc, lengthVxAcc]; | ||
} | ||
|
||
measureAllTrees(): number { | ||
/** | ||
* Measures the length of all trees and returns the length in nanometer and in voxels. | ||
*/ | ||
measureAllTrees(): [number, number] { | ||
const skeletonTracing = assertSkeleton(Store.getState().tracing); | ||
|
||
const totalLength = _.values(skeletonTracing.trees).reduce( | ||
(sum, currentTree) => sum + this.measureTreeLength(currentTree.treeId), | ||
0, | ||
); | ||
let totalLengthNm = 0; | ||
let totalLengthVx = 0; | ||
_.values(skeletonTracing.trees).forEach(currentTree => { | ||
const [lengthNm, lengthVx] = this.measureTreeLength(currentTree.treeId); | ||
totalLengthNm += lengthNm; | ||
totalLengthVx += lengthVx; | ||
}); | ||
|
||
return totalLength; | ||
return [totalLengthNm, totalLengthVx]; | ||
} | ||
|
||
measurePathLengthBetweenNodes(sourceNodeId: number, targetNodeId: number): number { | ||
/** | ||
* Returns the length of the shortest path between two nodes in nanometer and in voxels. | ||
*/ | ||
measurePathLengthBetweenNodes(sourceNodeId: number, targetNodeId: number): [number, number] { | ||
const skeletonTracing = assertSkeleton(Store.getState().tracing); | ||
const { node: sourceNode, tree: sourceTree } = getNodeAndTreeOrNull( | ||
skeletonTracing, | ||
|
@@ -684,9 +698,14 @@ class TracingApi { | |
const datasetScale = Store.getState().dataset.dataSource.scale; | ||
// We use the Dijkstra algorithm to get the shortest path between the nodes. | ||
const distanceMap = {}; | ||
// The distance map is also maintained in voxel space. This information is only | ||
// used when returning the final distance. The actual path finding is only done in | ||
// the physical space (nm-based). | ||
const distanceMapVx = {}; | ||
const getDistance = nodeId => | ||
distanceMap[nodeId] != null ? distanceMap[nodeId] : Number.POSITIVE_INFINITY; | ||
distanceMap[sourceNode.id] = 0; | ||
distanceMapVx[sourceNode.id] = 0; | ||
// The priority queue saves node id and distance tuples. | ||
const priorityQueue = new PriorityQueue<[number, number]>({ | ||
comparator: ([_first, firstDistance], [_second, secondDistance]) => | ||
|
@@ -699,16 +718,18 @@ class TracingApi { | |
// Calculate the distance to all neighbours and update the distances. | ||
for (const { source, target } of sourceTree.edges.getEdgesForNode(nextNodeId)) { | ||
const neighbourNodeId = source === nextNodeId ? target : source; | ||
const neightbourPosition = sourceTree.nodes.get(neighbourNodeId).position; | ||
const neighbourPosition = sourceTree.nodes.get(neighbourNodeId).position; | ||
const neighbourDistance = | ||
distance + V3.scaledDist(nextNodePosition, neightbourPosition, datasetScale); | ||
distance + V3.scaledDist(nextNodePosition, neighbourPosition, datasetScale); | ||
if (neighbourDistance < getDistance(neighbourNodeId)) { | ||
distanceMap[neighbourNodeId] = neighbourDistance; | ||
const neighbourDistanceVx = V3.length(V3.sub(nextNodePosition, neighbourPosition)); | ||
distanceMapVx[neighbourNodeId] = neighbourDistanceVx; | ||
priorityQueue.queue([neighbourNodeId, neighbourDistance]); | ||
} | ||
} | ||
} | ||
return distanceMap[targetNodeId]; | ||
return [distanceMap[targetNodeId], distanceMapVx[targetNodeId]]; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally remove the "." at the end or was this just by accident? I think a dot at the end makes sense as this is a complete sentence :)