Python Documentation bug in sitk.Image constructor

[dyollb]

Describe the bug
The documentation of SimpleITK.Image in the python API says

"""
        img:
        After the operation img is valid only for destructing and assignment;
        all other operations have undefined behavior.
"""

This seems to indicate that I should be very careful to not copy an image like so:

img_copy = sitk.Image(img)

I traced this constructor to following C++ code

SWIGINTERN PyObject *_wrap_new_Image__SWIG_1(PyObject *self, Py_ssize_t nobjs, PyObject **swig_obj) {
  PyObject *resultobj = 0;
  itk::simple::Image *arg1 = 0 ;
  void *argp1 = 0 ;
  int res1 = 0 ;
  itk::simple::Image *result = 0 ;
  
  if ((nobjs < 1) || (nobjs > 1)) SWIG_fail;
  res1 = SWIG_ConvertPtr(swig_obj[0], &argp1, SWIGTYPE_p_itk__simple__Image,  0  | 0);
  if (!SWIG_IsOK(res1)) {
    SWIG_exception_fail(SWIG_ArgError(res1), "in method '" "new_Image" "', argument " "1"" of type '" "itk::simple::Image const &""'"); 
  }
  if (!argp1) {
    SWIG_exception_fail(SWIG_ValueError, "invalid null reference " "in method '" "new_Image" "', argument " "1"" of type '" "itk::simple::Image const &""'"); 
  }
  arg1 = reinterpret_cast< itk::simple::Image * >(argp1);
  {
    try {
      {
        SWIG_PYTHON_THREAD_BEGIN_ALLOW;
        result = (itk::simple::Image *)new itk::simple::Image((itk::simple::Image const &)*arg1);

i.e., the copy constructor Image::Image(const Image&) is called.

To Reproduce
Look at SimpleITK.py in v2.3.0.

1. Operating system, version, and architecture

• OS: all
• Architecture: all

2. Programming language: Python 3
3. Version of SimpleITK: 2.3.0
4. How was SimpleITK installed?

• python -m pip install SimpleITK]

Expected behavior

The documentation should instead state that Image(img: Image) is a copy constructor, i.e. it copies the input image img.

Images
na

Additional context
na

[dyollb]

happy to provide a PR, if you agree that this is wrong/misleading.

[blowekamp]

Thank you for the report.

It looks like the C++ move constructor is what python picked up for the doc strings.
This is the file where SWIG picks up the doc strings:
https://github.com/SimpleITK/SimpleITK/blob/9db174c691899d0eb608edfeb649d50c215e7a25/Wrapping/Python/PythonDocstrings.i#L16417-L16441

That file is generated by manually running a script to convert the C++ Doxygen into the doc strings:
https://github.com/SimpleITK/SimpleITK/blob/master/Utilities/GenerateDocs/SwigDocUpdate.sh

This particular methods is not even seen by SWIG, but is enabled when the Doxygen is run. Maybe a separate doxygen run could be done with SWIG defined? Maybe one of those scripts could be updated which generate the .i file.

Honestly, this doctoring system is a little clunky. SWIG added some support for Doxygen strings, which would be easier, but last I looked at the output it appears inferior to what was currently being produced. It may deserve another look.

[dyollb]

If I understand correctly, the .i files are generated manually? Do you run it after each PR if the API changes or what triggers the update?

I think the simplest and cleanest way would be to define SWIG when generating the .xml files during the build. Then these methods probably would not get picked up by doxygen.

[blowekamp]

Actually, I think the easiest thing to do would to just ignore this method here:
https://github.com/SimpleITK/SimpleITK/blob/master/Wrapping/Python/Python.i#L33