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.

Sign up for GitHub

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

BUG: Fix off-by-one bug in end frame for 3D registration #127

Merged
merged 1 commit into from
Nov 21, 2024
Merged

BUG: Fix off-by-one bug in end frame for 3D registration #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions Hierarchical3DRegistration/Hierarchical3DRegistration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,15 +305,13 @@ def updateFrameSlider(self, CTSelectorNode: slicer.vtkMRMLNode):
"""Updates the slider and spin boxes when a new sequence is selected."""
if self.logic.autoscoperLogic.IsSequenceVolume(CTSelectorNode):
numNodes = CTSelectorNode.GetNumberOfDataNodes()
self.ui.frameSlider.maximum = numNodes
self.ui.startFrame.maximum = numNodes
self.ui.endFrame.maximum = numNodes
self.ui.endFrame.value = numNodes
maxFrame = numNodes - 1
elif CTSelectorNode is None:
self.ui.frameSlider.maximum = 0
self.ui.startFrame.maximum = 0
self.ui.endFrame.maximum = 0
self.ui.endFrame.value = 0
maxFrame = 0
self.ui.frameSlider.maximum = maxFrame
self.ui.startFrame.maximum = maxFrame
self.ui.endFrame.maximum = maxFrame
self.ui.endFrame.value = maxFrame

def onInitHierarchyButton(self):
"""UI Button for initializing the hierarchy transforms."""
Expand Down Expand Up @@ -500,7 +498,7 @@ def registerSequence(

try:
self.isRunning = True
for idx in range(startFrame, endFrame):
for idx in range(startFrame, endFrame + 1):
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering endFrame is set to the value of CTSelectorNode.GetNumberOfDataNodes(), using range(startFrame, endFrame) seems to be correct.

Indeed, considering the items in a sequence are retrieved starting with 0, calling self.autoscoperLogic.getItemInSequence(ctSequence, idx) with idx set to the number of item would not be correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also extending the list nodeList while it is being iterated over does not seem to be correct.

This pattern seems to be used in a few location

Copy link
Contributor

Choose a reason for hiding this comment

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

while it is being iterated over does not seem to be correct.

To follow-up offline discussion with @sbelsk, since this code was known to work, this will likely be reviewed/addressed in follow-up pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering endFrame is set to the value of CTSelectorNode.GetNumberOfDataNodes(), using range(startFrame, endFrame) seems to be correct.

Indeed, considering the items in a sequence are retrieved starting with 0, calling self.autoscoperLogic.getItemInSequence(ctSequence, idx) with idx set to the number of item would not be correct.

Makes sense, thanks for pointing out the inconsistency. The UI frame selector slider was set to go from minimum=0 to up to maximum=CTSelectorNode.GetNumberOfDataNodes() which would add one more frame than there really is.

I pushed a fix to make sure the maximum value the user is able to select is actually the last frame in the sequence. I believe all other places in the code use CTSelectorNode.GetNumberOfDataNodes() consistently such that the first frame is treated as index 0 and the last is treated as GetNumberOfDataNodes()-1. (E.g., error checks in TreeNode.py of the form if idx >= self.transformSequence.GetNumberOfDataNodes(): and the loop that goes for idx in range(self.transformSequence.GetNumberOfDataNodes()):).

nodeList = [rootNode]
for node in nodeList:
slicer.app.processEvents()
Expand All @@ -527,7 +525,7 @@ def registerSequence(

node.dataNode.SetAndObserveTransformNodeID(node.getTransform(idx).GetID())

if idx != endFrame - 1: # Unless its the last frame
if idx != endFrame: # Unless it's the last frame
rootNode.copyTransformToNextFrame(idx)
finally:
self.isRunning = False
Expand Down
Loading