InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

BUG: Provide an API for SpatialObjectProperties that works with python #4602

Closed aylward closed 2 months ago

aylward commented 2 months ago

The dictionary used by SpatialObjectProperties is queried using a function that returns the dictionary value in a variable that is passed by reference: https://itk.org/Doxygen/html/classitk_1_1SpatialObjectProperty.html#aff2b1c659a8e77bdb6afeef0bab037fb

bool itk::SpatialObjectProperty::GetTagScalarValue ( const std::string & tag, double & value ) const

This function is not correctly wrapped by SWIG, and cannot be used from within python.

Here is a snippet of code that demonstrates the issue:

In [1]: import itk
In [2]: so = itk.TubeSpatialObject[3].New()
In [3]: so.GetProperty().SetTagScalarValue("foo",1) In [4]: ret = 0.0
In [5]: so.GetProperty().GetTagScalarValue("foo",ret) 
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 so.GetProperty().GetTagScalarValue("foo",ret)
TypeError: in method 'itkSpatialObjectProperty_GetTagScalarValue', argument 3 of type 'double &'

This commit adds new functions that can be correctly wrapped. Old functions are unmodified to preserve backward compatibility.

N-Dekker commented 2 months ago

Thanks for your pull request @aylward It would be very disappointing if SWIG would still not support pass by reference or return by reference. I still hope that it can be enabled somehow. For pass by reference, StackOverflow Swig: How to wrap double& (double passed by reference)? suggested %include <typemaps.i> and some %apply statement. Did you already try that out?

aylward commented 2 months ago

@N-Dekker - I saw the same stackoverflow post, but I didn't pursue it because it changes the signature of the function when wrapping - causing the wrapped API to not match the C++ API. I didn't care for that break in philosophy compared to the rest of ITK's wrapping.

I am going to close this pull request and make another that contains only changes for the necessary Get statements. This pull request had other changes to make a consistent API (offering non-get versions of every function, even if the original wrapped as-is). Instead I'll reduce to the minimal changes for simplicity. I'll also mark the non-standard return by reference as functions to be deprecated. Stay tuned...