New Markups functionality retains unset control point positions

Development
1

I was trying out using the new landmark workflow outlined by @smrolfe here: Changes to the Markups Module

Specifically, manually create a set of labeled fiducial points on one case, then to use on the next case as a template, load the saved markup and unset all of the positions (by highlighting them on the control point list in the MarkupsModule and clicking the “Undefine position for the selected control points” button). The position coordinates disappear as expected, as do the glyphs. However, until new positions are chosen, the old point positions are still stored, and GetNthControlPointPosition() returns the old point position. I discovered this when I accidentally ran further processing without placing all the points. The measurements came out very odd, and I realized that there were measurements which involved a point which I hadn’t placed. This seems like a pretty serious bug. Once a position has been unset, requests for that point’s position definitely shouldn’t return an old position.

1 Like
2

Thanks, GetNthControlPointPosition() should be updated so it’s checking the point position status before returning the position. Could open a GH issue to document this?

1 Like
3

I thought about this too, it is nice that they remember their old position in case you mistakenly unset them by toggling the status icon - I do (one really wishes there was an undo/redo in markups). But the python interface should not return their old position, of course.

2 Likes
4

Posted this here:

1 Like
5

@ezgimercan the previous position will still be accessible using RestoreNthControlPointPosition(), but should not be returned through GetNthControlPointPosition().

6

To me, inability to get and set the position (temporarily making the position appear/disappear depending on some other properties of the control point) would be a much more serious bug. Why did you assume that the state of the control point blocks setting/getting the associated position value?

I see that crosstalk between position and status can is problematic. We had similar issues with “number of control points”. It is not obvious if the number includes skipped, previewed, undefined points or not.

I agree that something need to be done to make the API simple and bulletproof, but since markups classes got many features, offering a simple API has become a hard task.

7

We have introduced some crosstalk between position and status and various other methods, see:

Along these lines, we could add more flags to the GetNthControlPointPosition... methods.

However, the API is already quite complicated, and may not even fulfill the need of all use cases and doesn’t distinguish between all states (undefined, skipped, previewed, defined). So, I’m not sure that just adding a few flags would be sufficient.

8

What was a serious problem to me was that an old position was used when I thought it had been removed. This means that by missing a step (accidentally skipping a point that was supposed to be placed), and then running downstream processing, I got results that were almost reasonable, instead of getting an error or a more obviously unreasonable result. This happened because the position of the old point is not a terrible guess for the position of the new point (and more generally, the relationships among landmarks will be similar in the template case and the new case). This is a much more subtle kind of error than if unsetting positions reset them to be at (0,0,0) and not shown. If that happened, all distances between unplaced points would come out zero, ratios would throw errors, angles would have problems, etc. You would know something was wrong. In the current system, if you placed no points and ran your downstream processing you would get the exact same measurements as in the template case.

Anyway, now that I know that this is how it works, it is of course possible to add a check to make sure the point status is appropriate before using its position value. I think the primary reason I expected that the old position had been eliminated was that the position disappeared from the Markups module. If the position is just “dormant”, perhaps it would make sense to italicize and gray out the position? This might be confusing to users who expect it to disappear, but it better reflects the actual state of the point, and would make it clear that the unset position can be restored.

1 Like
9

Graying out position of undefined points is a great idea. It would eliminate a hidden state variable, thereby making the actual behavior of the software more clear for users and developers. It is also consistent with what is written to files.

2 Likes
10

Even in previous version of markups module, when we were recording “missing” landmarks as (0,0,0), I had added a check in my custom read functions (even if there is one, I would wrap it in a new one to add this check) where I set the coordinates of any point at (0,0,0) as (NA, NA, NA) or (None, None, None) depending on the downstream language. I was expecting something similar with the skip button and add empty point (+*) , rather than a 0,0,0 or retaining old position. But as I said, in the absence of an undo functionality, retaining old values make sense. I just updated my downstream readers in R and python to do this check and set the values to NA for missing landmarks.
I can imagine a scenario in other Slicer modules using markups, like PCA in SlicerMorph, this check again should be done if there is support for missing points. But if these downstream modules take the output of functions without any check, as in @mikebind 's scenario, they may produce incorrect results. So I am in support of updating these functions to return either an equivalent of NA or 0,0,0. Or update the API.

11

Actually, the undo functionality is already available in Slicer. You can enable undo/redo for fiducial nodes by copy-pasting these lines into the Python console:

# Enable undo for the scene

slicer.mrmlScene.SetUndoOn()

# Enable undo for markups fiducial nodes

defaultMarkupsNode = slicer.mrmlScene.GetDefaultNodeByClass("vtkMRMLMarkupsFiducialNode")
if not defaultMarkupsNode:
    defaultMarkupsNode = slicer.vtkMRMLMarkupsFiducialNode()
    slicer.mrmlScene.AddDefaultNode(defaultMarkupsNode)

defaultMarkupsNode.UndoEnabledOn()

# Add standard keyboard shortcuts for scene undo/redo

redoKeyBindings = qt.QKeySequence.keyBindings(qt.QKeySequence.Redo)
for redoBinding in redoKeyBindings:
    redoShortcut = qt.QShortcut(slicer.util.mainWindow())
    redoShortcut.setKey(redoBinding)
    redoShortcut.connect("activated()", slicer.mrmlScene.Redo)

undoKeyBindings = qt.QKeySequence.keyBindings(qt.QKeySequence.Undo)
for undoBinding in undoKeyBindings:
    undoShortcut = qt.QShortcut(slicer.util.mainWindow())
    undoShortcut.setKey(undoBinding)
    undoShortcut.connect("activated()", slicer.mrmlScene.Undo)

It is not enabled by default because historically it was attempted to be enabled for all nodes by default, which was too complicated. Since undo was disabled, developers did not pay attention to properly calling markupsNode->SaveStateForUndo() before all user actions, all those need to be added (without that, an undo reverts multiple user interactions), may be duplicated at a few places (requiring hitting undo/redo shortcuts multiple times to get a visible change), and of course some display update issues or other small bugs may be uncovered. Without these fixes, undo/redo will not feel very robust, but these should be all fairly easy to address.

If you want to further discuss of undo/redo then please create a new topic.

Both mask based (separate column specifies if an entry is missing or not) and special placeholder value based approaches are commonly used for dealing with missing values.

Slicer’s table infrastructure already supports both techniques. In table properties, the user can set any placeholder value as null value (0, -1, 1e6, nan, inf, …). This placeholder value is stored in the table schema.

For example, table exported from markups fiducial node:

image

Written to csv file (table.csv):

"label","r","a","s","defined","selected","visible","locked","description"
"F_1-1",-17.3607,-30.2059,-10.2143,1,1,1,0,""
"F_1-2",-12.0642,5.98722,-10.2143,1,1,1,0,""
"F_1-3",-67.678,6.9286,7.38225,0,1,1,0,""
"F_1-4",83.4364,-7.27105,-175.25,0,1,1,0,""
"F_1-5",-111.564,11.9882,-175.25,1,1,1,0,""

Schema file (Table.schema.csv) written out along with the table file:

"columnName","nullValue","type","componentNames"
"label","","string",""
"r","nan","double",""
"a","nan","double",""
"s","nan","double",""
"defined","","bit",""
"selected","","bit",""
"visible","","bit",""
"locked","","bit",""
"description","","string",""

Currently, markups module’s “Export/import table” feature uses only the masking technique (via a separate “defined” Boolean column), but we could add an option to replace point coordinates of undefined points with a null placeholder value.

This refactoring would be a good opportunity to switch to standard csv format from the limited and broken fcsv format (multi-line header, hardcoded columns, no way to specify column type, null and default values, etc.).

1 Like
12

PR issued to gray out the position when it is marked to skip placement