Big Image class overhaul -- #82, #114, #117, #123, #124, #125

[rmjarvis]

I think my big Image overhaul is ready for code review.

• The main thing is to implement the syntax that became known as option D in the "epic discussion" for issue worries about Image class #117.
• Also from worries about Image class #117: removed redefine method which Gary pointed out problematic, and Jim himself described as dangerous.
• Added setValue method as a way to set pixel values in python.
• Changed name of move method to setOrigin.
• Also added setCenter method for convenience.
• Changed semantics of copyFrom and all arithmetic ops to expect images to be the same shape, but not necessarily the same origin. The old behavior was to act on the overlap of the two bounds. The old behavior can be recovered by im[im.bounds & im2.bounds] = im2[im.bounds & im2.bounds] which is probably clearer as to the intent.
• Need to more easily specify the size of an image produced by draw() #82: I haven't separated out the image resize from the draw command, so this issue shouldn't be closed yet. But it does implement the basic syntax of draw(im,dx,wmult) in addition to im = draw(dx,wmult) in python.
• Add Python subimage assignment operator overloading #114: Added im[bounds] = im2 syntax for sub-images plus a bunch of unit tests to check this syntax does the right thing.
• Do we want to support mixed type Image operations? #123: The arithmetic in python now passes through to arithmetic on the numpy arrays, so mixed arithmetic works fine on the python side. I added this to the unit tests to check and all is well. I didn't implement it for C++. We can decide if that's worth doing or not. (I suspect we don't need it.)
• Image classes to multiply / divide by scalars #124: Added im * 2, im / 2, etc. And added units tests for these.
• Image division operators do not necessarily handle divide-by-zero nicely #125: Using numpy arithmetic means that there are no ugly C++ exceptions leading to crashes when we hit a division by zero. However, numpy doesn't throw a ZerDivisionError exception. For floats, it silently computes a Nan or Inf as appropriate. For ints, anything divided by 0 is 0. Weird, but we decided that was ok, since it probably won't ever matter.

[rmjarvis]

Minor correction about the new draw syntax. If you omit the image argument, then dx and wmult have to be specified as kwargs, not args. So:

im = sbp.draw(dx=dx, wmult=wmult)

This is because the arguments are actually:

sbp.draw(image=None, dx=0., wmult=1)

as suggested by Jim. If image is None, then a new image is created and returned. If image is provided, and has zero size, then it is resized appropriately and returned. If image is provided with non-zero size, then it is used as is and returned.

So you can omit the image argument, but then you have to specify the others by name.

[rmandelb]

Just a quick note: everything compiles fine and all tests pass on my machine. Will go through code etc. tonight / tomorrow...

[rmjarvis]

I forgot that I'd meant to implement the C++ arithmetic using composite classes to delay the calculation and thus save an inefficient copy. So I just added this functionality. This method also makes it easy to implement the mixed-type arithmetic, so I did that too.

I haven't written any C++ unit tests for this yet, but I really should, since the python unit tests don't test these functions considering that the python layer implements the arithmetic using numpy, rather than calling the C++ functions.

[rmandelb]

Hi Mike -

Barney and I just went through part of this. First, a few comments:

• We had not initially expected that there would be a new BaseImage class, but it does seem to make sense as it allows you to define all the functions that don't change images in one place, rather than attaching them to both Image and ImageView and having to maintain code in both places.
• Likewise we had been thinking primarily in terms of Image and ImageView as discussed on worries about Image class #117, but we see the distinction you're making by having an additional ConstImageView which does seem to have its own purpose. I worry a little about the proliferation of Image-type classes (mainly in terms of the confusion that might arise for developers who need to get into the guts of the C++ code, rather than for python end-users), but I also don't have a clear solution that would let us easily eliminate one of these.

A question which we're happy to discuss more on the phone - I'm putting this here mainly so others have a chance to chime in, but if we talk about it on the phone then one of us can summarize the answer here laterL

We had thought that the choice of which is an Image and which is an ImageView would allow python users to mainly interact with Image. But we noticed that it is no longer possible to initialize an Image from a numpy array, and instead this is only possible for ImageView. Why?

More later -
Rachel

[rmandelb]

@joergdietrich , would you be willing to try compiling and running the tests on "the little Linux box that could (find subtle system-dependent issues)"? :)

@gbernstein and @TallJimbo , given the size of this overhaul, we'd like your feedback before merging.

[rmandelb]

Barney, Mike, and I just got off the phone, and I'm going to try to summarize the answers to the questions / comments above (but please correct me if I get something wrong...):

1. ConstImageView is not really necessary at the python level. While it is currently wrapped, it could (should?) be removed. Basically, it wouldn't have the methods for changing image values in-place (e.g., +=) but it is likely that the values of the numpy array could be directly changed.

2. Our concern about having to initialize an ImageView (as in the unit tests) and the implications for python users: if you look at Demo.py, actually you will see that it has not changed at all, the existence of ImageView is not something that a user would notice. And likewise if we want to use our base classes and employ the draw method that takes an image argument, that's an Image. So for users, the current setup should be fine in terms of Image being the main thing they interact with (developers will of course have to know about both).

[joergdietrich]

I have one error:

======================================================================
ERROR: Test that subImages are accessed and written correctly.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/nose/case.py", line 183, in runTest
    self.test(*self.arg)
  File "/home/joerg/src/GalSim/tests/test_Image.py", line 542, in test_Image_subImage
    image[bounds] = galsim.ImageView[types[i]](100*sub_array)
ValueError: numpy.ndarray argument has incorrect data type

BTW, this little Linux box will stay behind in Michigan when I will move to Munich in June.

[rmjarvis]

Joerg, I think I might have fixed the error you found. Please git pull and try again.

[joergdietrich]

The error persists.

[barnabytprowe]

Shame that little Linux box won't still be around come June; it's useful...!

I did my own test, on the constness of the ConstImageView.array attribute. For me at least, this is working:

In [1]: import galsim

In [2]: import numpy as np

In [3]: im = galsim.ConstImageViewD(np.ones((3,3)))

In [4]: im.array
Out[4]: 
array([[ 1.,  1.,  1.],
       [ 1.,  1.,  1.],
       [ 1.,  1.,  1.]])

In [5]: im.array[0, 1] = 5.
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
/Users/browe/great3/GalSim/<ipython-input-5-cca19ea4f9c4> in <module>()
----> 1 im.array[0, 1] = 5.

RuntimeError: array is not writeable

So I think that what is implemented is working correctly, independently of whether we want to keep the ConstImageView available at the Python layer. I can't think of a very strong reason to be rid of it... We might want to label some of our internals as inviolable and produce a runtime error Exception if anyone attempts to change them (the OpticalPSF lookup tables for instance, or the RealGalaxy images from COSMOS).

What I will do, however, is add a quick unit test just to check that we are getting a RuntimeError if attempting the above on all systems - would be nice to know that this behaviour is consistent!

[rmjarvis]

I'm not sure why that error is persisting. Is it on the same line still?

Anyway, I added some diagnostic information to be printed along with the error to help figure out what the conflict is exactly. So please do another git pull and tell me what the error is now.

[rmjarvis]

I think Jim's comment was probably right. And after committing a change that might have worked, I actually decided to go with a whole different tack, which is to switch the instantiation types of Image from short and int to int16_t and int32_t.

I was thinking about why we might want to have integer Images, and it's probably mostly to be compatible with reading such from fits files. In that case, we will want the image to match up with the type in the fits file, which are almost certainly going to be 16 and 32 bit integers. So the right solution is to force C++ to use those sizes too, rather than whatever it might call an int.

I'm hoping that the python's int16 and int32 will always match C++'s int16_t and int32_t, but I can't say that I have a huge amount of confidence in this fact, so I guess we'll see. :) Anyway, give it a try Jörg, and see if this works for you.

[rmjarvis]

And if this doesn't work, I'm fine with merging and making a new issue about it. No sense it holding up other work until this gets straightened out.

[joergdietrich]

We keep progressing from bad to worse. Now it doesn't even build:

g++ -o pysrc/.obj/SBProfile.os -c -O2 -fno-strict-aliasing -g3 -Wall -Werror -fPIC -Iinclude/galsim -Iinclude -Iinclude -I/usr/include/python2.6 -I/usr/lib/pymodules/python2.6/numpy/core/include pysrc/SBProfile.cpp
pysrc/Image.cpp: In function ‘boost::python::api::object galsim::<unnamed>::getNumPyType() [with T = int]’:
pysrc/Image.cpp:399:   instantiated from here
pysrc/Image.cpp:32: error: incomplete type ‘galsim::<unnamed>::NumPyTraits<int>’ used in nested name specifier
pysrc/Image.cpp: In static member function ‘static boost::python::api::object galsim::<unnamed>::PyImage<T>::getArrayImpl(boost::python::api::object, bool) [with T = int]’:
pysrc/Image.cpp:96:   instantiated from ‘static boost::python::api::object galsim::<unnamed>::PyImage<T>::getConstArray(boost::python::api::object) [with T = int]’
pysrc/Image.cpp:243:   instantiated from ‘static boost::python::api::object galsim::<unnamed>::PyImage<T>::wrapImage(const std::string&) [with T = int]’
pysrc/Image.cpp:399:   instantiated from here
pysrc/Image.cpp:74: error: incomplete type ‘galsim::<unnamed>::NumPyTraits<int>’ used in nested name specifier
pysrc/Image.cpp: In static member function ‘static void galsim::<unnamed>::PyImage<T>::buildConstructorArgs(const boost::python::api::object&, int, int, bool, T*&, boost::shared_ptr<T>&, int&, galsim::Bounds<int>&) [with T = int]’:
pysrc/Image.cpp:187:   instantiated from ‘static galsim::ConstImageView<T>* galsim::<unnamed>::PyImage<T>::makeConstFromArray(const boost::python::api::object&, int, int) [with T = int]’
pysrc/Image.cpp:374:   instantiated from ‘static boost::python::api::object galsim::<unnamed>::PyImage<T>::wrapConstImageView(const std::string&) [with T = int]’
pysrc/Image.cpp:406:   instantiated from here
pysrc/Image.cpp:107: error: incomplete type ‘galsim::<unnamed>::NumPyTraits<int>’ used in nested name specifier

scons: *** [pysrc/.obj/Image.os] Error 1
scons: building terminated because of errors.

[rmjarvis]

Well, in general I call that progress. I'd much rather find problems at compile time than have the compile work, but let problems lurk until some random time at run time.

But anyway, this means that npy_int32 is not the same type as int32_t. One's probably a typedef of long and the other of int. That's unfortunate. I was hoping the numpy people would have make those sync up, but I guess not. Anyway, I've now instantiated the NumPyTraits struct for int32_t rather than npy_int32, so it should compile now. I guess we'll see if it runs.

[rmjarvis]

And we probably need to fill the Image dictionary with the npy_ types, so I just did that too. Which makes me think that I probably need to put the Normalize stuff back in. So hold off a couple minutes before trying this.

[barnabytprowe]

That is weird, I don't get the compilation error...

[rmjarvis]

It would only be on systems where int == long. Unfortunately, I don't have one of those to test on...

[rmjarvis]

OK, give it a try now.

[joergdietrich]

another build error:

g++ -o pysrc/.obj/SBProfile.os -c -O2 -fno-strict-aliasing -g3 -Wall -Werror -fPIC -Iinclude/galsim -Iinclude -Iinclude -I/usr/include/python2.6 -I/usr/lib/pymodules/python2.6/numpy/core/include pysrc/SBProfile.cpp
pysrc/Image.cpp: In function ‘boost::python::api::object galsim::<unnamed>::getNumPyType() [with T = long int]’:
pysrc/Image.cpp:416:   instantiated from here
pysrc/Image.cpp:49: error: incomplete type ‘galsim::<unnamed>::NumPyTraits<long int>’ used in nested name specifier
scons: *** [pysrc/.obj/Image.os] Error 1
scons: building terminated because of errors.

[rmjarvis]

OK. Here's another attempt. I got rid of the npy_* types entirely from the C++ side.

[joergdietrich]

Builds and passes all tests. Thanks a lot Mike.

[rmjarvis]

Phew. Finally. :) Good.

[rmandelb]

Fantastic. Let's leave it overnight for any further comments, and then we can merge in the morning.

[rmandelb]

Barney just resolved some comments, and I've confirmed that everything still builds and all tests pass, so I'm going to merge now. Thanks, Mike, for the tremendous job on this significant reworking of the Image class! (and I think you win the award for "most issues resolved with a single pull request")

I'll go through and close / comment on the various issues as appropriate.