How to deal with HoughTransform2DLinesImageFilter GetLines() method in python

[nick]

@Niels_Dekker

@nick

I’m not sure if modify the method to return a copy of the object is a good idea: I think it could break some code developed by other users… is there a way in python to return a copy of this kind of objects?

At the client side: sure, you could create a new Python list inside your RunHough function, and copy each line object (which is if fact a smart pointer) into your list, which you will then return. That should work just fine.

At the Python wrapper side: it might be possible to do return “by value” for GetLines(), even when the C++ code says “by reference”. But personally I wouldn’t really like to introduce such an inconsistency between the C++ API and the Python wrapping.

At the C++ side: theoretically it might break some C++ user code when the list of lines is returned by value, instead of by reference, but I don’t expect that it will affect many users. What particular use case do you think about?

I wasn’t thinking of any specific case… mine was just a theoretical statement.

[Niels_Dekker]

@nick I have to admit, your theoretical statement is correct

Still, you may have a look at my new pull request, that could solve the issue (your segfault) in practice:

BUG: HoughTransform2D GetLines(), GetCircles() return ListType by-value

Do you think it’s OK?

[fbudin (Francois Budin)]

Thanks @nick for figuring this out, and thanks @Niels_Dekker for working on a PR. We have seen this sort of problems appear a few times now in Python:

• https://github.com/InsightSoftwareConsortium/ITK/pull/951
• https://github.com/InsightSoftwareConsortium/ITK/commit/fede4b865d2ea7a4fdde0d81fc496b7054e8cfbd

I would love to find a better solution than the manual fix we have been using so far, since ITK uses return by reference quite a bit, but I haven’t found that yet… Ideally, we would want the C++ code to stay the same, and the wrapping to be modified to return by value, similar to what the two commits/PR that I listed above do.

Note: Interesting read that may or may not be helpful to detect the classes that need to be updated: http://swig.10945.n7.nabble.com/Gotcha-with-returning-a-const-reference-to-an-object-td12200.html

[Niels_Dekker]

Thanks for your feedback, Francois.

@fbudin

Ideally, we would want the C++ code to stay the same, and the wrapping to be modified to return by value, similar to what the two commits/PR that I listed above do.

When the C++ code returns by const reference, it might be OK for the Python wrapper to return by value. But when the C++ code returns by non-const reference, I find it counter-intuitive for the Python wrapper to return by value. It would introduce an inconsistency between Python user code and the equivalent C++ user code.

The decision whether or not the Python wrapper should return by value might also depend on the size of the object.

Clearly this is a recurring problem, but I think there is no silver bullet.

[fbudin (Francois Budin)]

@Niels_Dekker: We indeed may need to look at each individual function need to be updated to return by value and which one should still return by reference. The const correctness is ignored/removed by SWIG, so in the end, sadly, that will not help. What I hope we will be able to do, is fine a way to detect these functions automatically, and maybe have a whitelist of the ones that should still return by reference. Based on my experience with ITK Python, it does seem that returning by reference causes a lot of issues, so I anticipate that most functions would need to be somehow modified (or wrapped) to actually return by value, and that only a few would still return by reference in Python, when absolutely necessary. But this is more of an item to put on a to-do list. For now, we will have to address this issue on a case-by-case basis. Thanks for your PR!