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.

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: Filter not accepting mesh input for Python wrapping #10

Closed
tbirdso opened this issue Mar 14, 2021 · 5 comments · Fixed by #11
Closed

BUG: Filter not accepting mesh input for Python wrapping #10

tbirdso opened this issue Mar 14, 2021 · 5 comments · Fixed by #11

Comments

@tbirdso
Copy link
Contributor

tbirdso commented Mar 14, 2021

Overview

MeshProcrustesAlignFilter builds and is wrapped successfully for template arguments of type itk::Mesh<double, 3>. However, attempting to set input meshes in Python following setting number of inputs returns an error:

>>> filter.SetInput(0,m1)
...
TypeError: in method 'itkMeshProcrustesAlignFilterM3M3_SetInput', argument 3 of type 'itkMeshD3_Pointer'

At face value it seems that there is a type mismatch between the expected argument of type itkMeshD3_Pointer and the given value of type itkMeshD3. However, per Discourse discussion Python is not expected to handle pointers directly.

Steps to Reproduce

  1. Read in valid ITK::mesh<double,3> in Python and validate type:

m1 = itk.meshread('C:/users/tom.birdsong/source/repos/hasi/test/input/901-L-mesh.vtk', itk.D)

type(m1)
<class 'itk.itkMeshBasePython.itkMeshD3'>

  1. Instantiate filter and validate type:

filter = itk.MeshProcrustesAlignFilter[itk.Mesh[itk.D,3], itk.Mesh[itk.D,3]].New()

type(filter)
<class 'itk.itkMeshProcrustesAlignFilterPython.itkMeshProcrustesAlignFilterM3M3'>

  1. Set number of filter inputs (for example, trying to find the mean of two meshes)

filter.SetNumberOfInputs(2)

  1. Attempt to set first mesh input

filter.SetInput(0,m1)

Expected behavior

Setting inputs up to the allocated number in Step 3 succeeds without error.

Observed behavior

Traceback (most recent call last):
File "", line 1, in
TypeError: in method 'itkMeshProcrustesAlignFilterM3M3_SetInput', argument 3 of type 'itkMeshD3_Pointer'

@tbirdso
Copy link
Contributor Author

tbirdso commented Mar 15, 2021

@thewtex @dzenanz Could you add insight as to how pointer arguments might be causing issues in Python here? Is it likely an issue with the module wrapping, or a symptom of a different root cause?

@dzenanz
Copy link
Contributor

dzenanz commented Mar 15, 2021

In 2018, mesh support in Python was deficient (InsightSoftwareConsortium/ITKSphinxExamples#66). Maybe not everything has been updated properly in ITK? If you try updating the example in InsightSoftwareConsortium/ITKSphinxExamples#66 maybe you could get a better grip of mesh support. Trying it out on a simple, stand-alone example should be helpful for this custom filter.

@tbirdso
Copy link
Contributor Author

tbirdso commented Mar 15, 2021

I'll check it out, thanks!

@thewtex
Copy link
Contributor

thewtex commented Mar 16, 2021

@tbirdso The SetInput method currently has a Mesh SmartPointer as its argument:

https://github.com/slicersalt/ITKMesh3DProcrustesAlignFilter/blob/091fb7c4998fd9182108adbfe0275fa764dca453/include/itkMeshProcrustesAlignFilter.h#L104

This needs to be a raw pointer instead of a SmartPointer. Passing a smart pointer causes unnecessary smart pointer construction / destruction, and wrapping and wrapping ownership rules are set up for raw pointers.

@tbirdso
Copy link
Contributor Author

tbirdso commented Mar 16, 2021

Thanks @thewtex. I wasn't understanding that Pointer = SmartPointer< Self >, but now I see that in the itk::Object documentation as well.

I found Cpp guidelines to help myself better understand the issue with this design pattern: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-smart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants