worries about Image class

[gbernstein]

There are a couple of things about the C++ Image class that might be sources of future problems and I want to poll you all to see if the current behavior is viewed as a desired feature. The issue is that the pixel data of the class are shared whenever the image is copied or assigned, but once two Images (or subimages) share pixels, there is no enforcement that they continue to share their information about pixel boundaries and scales. The old Image class shared the ancillary information with copies, not just the pixel array.

Sharing the pixel array is essential, so that functions may return Image objects without placing huge arrays on the stack. But I am nervous about "partial sharing" of the objects' meaning, because it might be confusing to have two different objects in the code that have different interpretations of what the pixels mean. Or do you all consider this a feature, that you can have two Image objects that are assigning different pixel coordinates to the same data?

In any case the resize() method is potentially troublesome as written, as it sometimes severs its relationship with the original pixels and sometimes does not. So when some routine copies your image and resizes it, you are not going to know whether this new object is still sharing pixels with your original one. I suspect that for sanity, the code should be changed so that it always allocates a new array if there are any other instances sharing the data (currently it does not if the array size is not changing).

The redefine() method is described as "dangerous" - is there any case when this is needed? Would it be better to just make people get themselves a new Image?

[TallJimbo]

I agree with your recommendation about resize, that's a good catch and the current behavior is clearly bad.

redefine can probably be removed (or made private; I think some other member functions may use it). I made it public when I didn't fully understand the behavior of the old image class and I was trying to make sure the new one could do everything it needed to do.

I think pixel boundaries cannot be shared, because as soon as you make a subimage the parent's boundaries no longer match the original. We could easily throw an exception if the boundaries of an image that shares data are shifted (or if the scale is changed), but these might be useful operations as they allow us to change the coordinate system of a subimage (because there may be more than one useful coordinate system for a particular image).

[rmjarvis]

Gary's concerns are similar to the ones I raised in the comments for #114. I think the API for the current Image class is pretty confusing and not very intuitive, specifically because of issues like this with shallow copies. So I think we should really have an extensive review of what functionality and behavior we want for Image. Jim changed the behavior pretty significantly with his overhaul, and I don't think the new version was ever really code reviewed.

In particular, I think most of the problems come from the desire to be able to return Images by value. Do we really need this functionality? Can't we just always provide an image object to be edited rather than ask for images to be created for us and returned? Then we could switch to using deep copies, which have the intuitive behavior that I think we want. (See my code example in the #114 comments for an example of where shallow copies are very confusing.)

[rmjarvis]

I'll add some food for thought about a possible API that I think might make more sense, which is to mimic the behavior of the Matrix and MatrixView classes in TMV.

• A Matrix always owns its own data, so copies are deep: Matrix<T> m1 = m allocates new memory for m1 and copies the values. m1 can be resize, which will reallocate new space if necessary.
• A MatrixView does not own its own data and copies are shallow: MatrixView<T> m2 = m.view() just creates a view into the data owned by m. No allocation, and changes in one are reflected in the other. m2 cannot be resized.
• A ConstMatrixView is like a MatrixView, but is read-only. ConstMatrixView<T> m3 = m.view()

I think this three-class model captures all of what Jim was trying to accomplish with the Image<const T> and Image<T> distinction, but adds an additional distinction between images that own their own data and images that don't, which would make clear when things like resize are allowed and which images share data with each other.

[TallJimbo]

Mike's API is feasible. I originally avoided it because I thought it added too much of a burden on classes and functions that want to operate on images:

• Any class or function that needs to operate on an image in a read-write sense needs to have two overloads (Image, ImageView).
• Any class or function that needs to operate on an image in a read-only sense needs to have three overloads (add ConstImageView, though we might be able to get around this via inheritance as I did with Image<T> and Image<const T>).

Overall, I think I'm more comfortable with the way the current Image class works because it is very similar to how NumPy arrays work, and I've had a long time to get comfortable with that (I'm quite willing to admit that NumPy arrays are somewhat counterintuitive, even in Python). The mapping to C++ is admittedly very counterintuitive, but it's the product of a lot of experimentation in my own ndarray library - I spent a lot of time rewriting that every few months a few years ago, and it only stabilized once I switched to something very similar to what I put in the image classes. That's not a statement that it's the only valid design (and it's less problematic there because I don't have any coordinate system information packaged with an array), but there are a lot of good C++ designs that don't work well with NumPy. I think it's worth exploring other options, but we need to make sure the NumPy semantics make sense.

[TallJimbo]

I should also say that I do definitely share the concerns Mike raise on #114 about the operators, and I think we should make fixing that a priority.

I took a different approach to that in ndarray than Mike took with TMV, but IMO either one is better than what we have now from a usability standpoint. But we should consider that what we have right now might be better from a maintainability standpoint - I think any satisfactory solution would add a lot of code to the Image class without really changing the Python interface much, so that judgment depends on how big the code using Image from C++ is relative to how big the Image implementation itself is.

If we don't do a big overhaul, however, we should take a look at the operators in Python and make sure they aren't similarly confusing. If they are, we could consider just removing them, as we can just delegate to the NumPy operators.

[rmandelb]

I am open to an overhaul as I think sufficient motivation has been presented that some change is necessary (though I am on the fence as to whether we need an overhaul, or whether minor changes like dealing with problematic functions and Python operators would suffice). I would just like to raise a question of timing: do you think this is something that needs to be done for the 2nd milestone, or if it could wait for a cleanup period after this milestone? My big concern is that if we plan on writing lots of code to handle multi-object outputs, that might have to be changed in a significant way if the Image behavior changes, which would suggest that we should make this change now and perhaps extend the time period of the 2nd milestone to accommodate this extra work.

[gbernstein]

So here are several possible levels of intervention:

1. Jim fixes the resize() issue, otherwise stay the same with the convention that each instance has its own independent labeling of the pixel coordinates, and resizing always disconnects any sharing.

2. Make the bounds and pixel scale immutable. Very simple to code and use, but might be inefficient as would require a copy of the data array to a new instance when you want to relabel pixel coordinates.

3. Share bounds and pixel scales, do some extra bookkeeping on subimages or just disallow changing pixel labelling on subimages.

4. Recode to have "Views" as for TMV.

I would vote for (1) or (2), they'll work with no or minimal change to current use. (4) would definitely work but is more effort than I think is needed here (and also Jim noted requires more effort on all the client codes). (3) would be trickier but also not worth the effort.

I do think it is important to be able to return an Image() from a function instead of always having to create one to pass in as an argument and get it filled in. It makes the client code simpler. And one of the virtues of SBProfile is that it will choose appropriate pixel scale and image sizes for you, making this usage possible & convenient.

[rmjarvis]

Jim wrote:

Mike's API is feasible. I originally avoided it because I thought it added too much of a burden on classes and functions that want to operate on images:

• Any class or function that needs to operate on an image in a read-write sense needs to have two overloads (Image, ImageView).
• Any class or function that needs to operate on an image in a read-only sense needs to have three overloads (add ConstImageView, though we might be able to get around this via inheritance as I did with Image and Image).

For the first one, one solution is to define everything in terms of ImageView and then also have a second function that takes a non-const Image and trivially call the former with im.view(). This is the solution TMV uses. It's not too burdensome in practice, since the extra function is a one-liner.

For the second one, you can either derive all three class from a common base class (ConstImage perhaps) and use that as the parameter type for your functions (the solution TMV used prior to v0.70), or define things in terms of ConstImageView and let all three classes trivially convert to that (the solution TMV uses now). Either way, you only need to write a single function for anything operating on a read-only image.

Gary wrote:

I do think it is important to be able to return an Image() from a function instead of always having to create one to pass in as an argument and get it filled in. It makes the client code simpler. And one of the virtues of SBProfile is that it will choose appropriate pixel scale and image sizes for you, making this usage possible & convenient.

The automatic sizing happens for the pass by reference version of draw too. In fact, the entirety of the draw function that returns an Image by value is

Image<float> SBProfile::draw(double dx, int wmult) const
{
    Image<float> img;
    draw(img, dx, wmult);
    return img;
}

Are we really saying that we can't just declare the image first and then pass it by reference? That does't strike me as a big imposition on the client code.

[pmelchior]

I agree with Mike on the pass-by-reference issue. It makes the code a little less user-friendly, but also programmatically cleaner. The setup of things like pixel scale is something we can expect from the user to do.

On the original issue how to deal with deep and shallow copies of Image I'm very much in favor of Mike's solution in TMV, but it seems that the python mapping would not go well with that approach.
Is there something we want to make use of in the future that really requires a complete overhaul? If so, I would address the problem now and not build code around it that we would have to change later on. If not, I think it is acceptable to follow Gary's suggestions 1) or 2), but we then need to keep these limitations in mind.

[rmjarvis]

Jim or Peter,

Could you explain in detail why you think the python version of Image wouldn't work well with this kind of approach? It's not obvious to me what is different or hard. And I actually care more about the python layer being clear than the C++ layer, since that's the code that the end user is going to interact with.

Both of you are more expert than I am wrt python, so I'm willing to believe you. But I'd like to understand the difficulty you're seeing.

[TallJimbo]

Mike, I do think we could make the Image + View idea work with NumPy, and the idea is growing on me; I just haven't had time to think about it deeply yet and I've been burned in the past. I don't think we could really make the Image class use copy-on-write in that case, but that's probably not a big deal. However, I also think we might be able to get away with essentially just the View objects. When I think about what we want to do with Images and Views, it seems like the vast majority of the functionality goes in the View classes, and all we really want from the Image class is a safer, less confusing way to "reset" some of the properties of a view.

So, here's a start at a concrete proposal somewhere between Gary's (2) and (4):

The current Image classes are very close to the View concept Mike brought up. If we remove resize, make the bounds and pixel scale immutable, and additionally make the assignment operator deep rather than shallow, I think they'd be exactly what we'd want a View class to be. This solves the confusion with the assignment and augmented assignment operators that Mike brought up in #114, and it also addresses Gary's concerns with subimages changing coordinate systems. And I don't have any concerns about whether it still works well with NumPy.

That leaves us with two problems:

• The copy constructor (shallow) and assignment operator (deep) don't have the same semantics. I'm starting to think this is the least of all the evils, and it's best to live with it. Many of the problems I've encountered due to that before were SWIG-related, and we don't have to worry about them here.
• If all we have are the View-like Image classes, we don't have anything we can pass "empty" into a draw routine and have the draw routine resize and allocate it. I think there are few solutions to this:
  • We could add a true deep-copy Image class and rename the current classes to ImageView. We could make this very minimalist, so that all you can do with an Image would be to get a view from it and resize/shift it; I'm pretty sure that would work with NumPy. But this would still entail duplicating the signatures of all the draw functions, which isn't pretty in Python where overloading isn't easy to express.
  • We could add a shallow reset method to the now View-like Image classes, which would essentially do what the assignment operator does now, but with a hopefully less-confusing syntax. This re-raises some of the concerns we wanted to address, but I think it mitigates them quite a bit.
  • We change the signatures of the draw functions to accept a View-like Image by pointer and return it. When the pointer is null, we allocate create and return new Image, and when it's not null we return a shallow copy after modifying the input in-place. The downside of this is that the draw routines wouldn't be able to return something else (like the flux they currently return), but the advantage is that is translates to a very nice Python syntax, in which we use None instead of an empty Image, and can use the same signature to modify an image in-place or return a new one. To be more precise, here's how a draw function would look in C++:

Image<T> draw(Image<T> * output = NULL, ...) {
    Image<T> result = output ? Image<T>(*output) : Image<T>(getDrawSize());
    ...
    return result;
}

and here's how you'd call it in Python:

image1 = draw(...)  # default argument is None -> NULL
image2 = ImageD(...)
draw(output=image2, ...) # return value is the same as image2, but we can ignore it.

[pmelchior]

Jim,

this sounds very reasonable. I would definitely prefer a deep assignment operator, and, as you pointed out, this would immediately get rid of some confusion.

Regarding draw: Coming from C++, I would prefer solution 1 (the true-deep Image), but I understand your concerns of signature duplication. That said, solution 3) involves a pointer that I would rather avoid, but it is elegant and easy to use from the python side.

[rmjarvis]

What is the problem with the overload in python? This seems like a case where the overload resolution is easy, since the signature is the same, just the type is changing. Since python uses duck typing, it will accept either one. Then you pass it along to the C++ draw function which actually does do the overload resolution.

It would be even easier if we separate out the automatic resize functionality from the actual drawing step. Then the python version could be written as:

def draw(self, image, dx=0., wmult=1):
    # Omitting checks for type of dx, wmult
    if image.isinstance(galsim.Image) and not image.getBounds().isDefined():
        self.SBProfile.autoResize(image)
    self.SBProfile.draw(image.view(), dx=dx, wmult=wmult)

Then the SBProfile version only needs to be defined in terms of an ImageView. But the python version would preserve the automatic resize feature for real Image's, but not ImageView's.

[barnabytprowe]

Hello all, I've not weighed in on this up until now, feeling (largely) totally unqualified to comment on most of the issues that have arisen :) It's been instructive though...

I agree that assignment vs copy operator semantics (deep vs shallow) is not a terrible evil: the fact that it can be expressed relatively simply is already a big plus point.

As regards draw(), how much of the SBProfile code makes use of the flux value currently returned by the SBProfile::draw method with that particular signature...? I'm going to dig around...

[gbernstein]

On Apr 24, 2012, at 6:52 PM, Barnaby Rowe wrote:

As regards draw(), how much of the SBProfile code makes use of the flux value currently returned by the SBProfile::draw method with that particular signature...? I'm going to dig around…

Probably very little and easily worked around. I added that return value mostly because I could, and it had no disadvantage.

[barnabytprowe]

Thanks Gary! So far my investigations (using, admittedly, a blunt tool like grep -r ".draw(" *) have revealed not one case of it's use so far, but that's hardly a definitive statement...

Mike: I'm afraid I don't have a definitive answer, but I know that the C++ wrapping in Boost.Python can be picky about getting the right type of argument supplied for each wrapped function. I'm probably expressing this poorly, but in order not to duplicate all the draw calling signatures I think we might have to think of a clever way to handle this in C++ without the Python wrappers "knowing" (or rather, caring) about the difference between an Image and an ImageView... No idea if this is possible...

[TallJimbo]

Mike's example would work fine; by "overloading isn't pretty in Python" I just meant "you have to write if statements with isinstance checks". It's not horrible by any means.

But I don't see that adding a deep-semantics Image class in addition to the shallow-copy/shallow-assign view class adds that much value in C++ relative to using pointers or a reset method, and in Python it seems like having two classes is essentially unnecessary.

[rmjarvis]

The value of having both Image and ImageView in both C++ and python is in clarity. You always know who owns the data, and you can very easily declare whether or not you want to duplicate data:

im2 = Image(im1) # Copies the data
im2 += sky_image # Only im2 is affected
im3 = ImageView(im1) # We are just making a view of im1's data
im3 += sky_image # Both im3 and im1 are affected

This seems like a useful distinction in both python and C++. I think the only difference between the C++ and python syntax is the meaning of

im4 = im1

In python, this is always just a rename, so basically a shallow copy, whereas in C++ we can make it a deep copy. I do find the python behavior confusing, but it's a basic confusion of the python language, not something specific to this discussion.

[barnabytprowe]

Hi all,

As this is important and relevant to the Milestone Issues #38 and #82 (among others), and more generally, Rachel and I have 'upgraded' this into also being a Milestone Issue :) In light of this, we've also pushed the Milestone deadline back a week; we think this is important.

We're going to email the code list about all this too. In the next couple of hours (I need to get the bus into work now ;) I will summarize the options presented here so far for a final discussion, poll and plan for implementation. All this will happen on this Issue discussion, so watch this space! Any further ideas, solutions hybrids etc. that people want to present, feel free and I'll make sure they get in the summary...

[barnabytprowe]

OK, so here's an attempt at a summary of options that have gained some measure of popular support (using capitalized latin characters due to the mild profusion of other enumerators already on this list!). These seem to me to be the main ones, but please add more if you feel I've missed something that has been discussed:

A) Gary's first "conservative" option:

Jim fixes the resize() issue, otherwise stay the same with the convention that each instance has its own independent labeling of the pixel coordinates, and resizing always disconnects any sharing.

B) Gary's second "conservative" option:

Make the bounds and pixel scale immutable. Very simple to code and use, but might be inefficient as would require a copy of the data array to a new instance when you want to relabel pixel coordinates.

C) Gary's option 3, not discussed very much:

Share bounds and pixel scales, do some extra bookkeeping on subimages or just disallow changing pixel labelling on subimages.

D) Basically Gary's option 4, combined with the first 'flavour' of Jim's proposed solution, which I think is being most strongly advocated by Mike based on his experience with TMV.This seems like it would be in the form of a Image and ImageView classes at the Python level.

As Jim says:

The current Image classes are very close to the View concept Mike brought up. If we remove resize, make the bounds and pixel scale immutable, and additionally make the assignment operator deep rather than shallow, I think they'd be exactly what we'd want a View class to be. This solves the confusion with the assignment and augmented assignment operators that Mike brought up in #114, and it also addresses Gary's concerns with subimages changing coordinate systems. And I don't have any concerns about whether it still works well with NumPy.

Example usage given by Mike would be:

im2 = Image(im1) # Copies the data
im2 += sky_image # Only im2 is affected
im3 = ImageView(im1) # We are just making a view of im1's data
im3 += sky_image # Both im3 and im1 are affected

Mike also points out that if we can separate out the automatic resize functionality from the actual drawing step of the draw() method, then this might be implemented to handle both types easily while letting SBProfile deal only with Views:

def draw(self, image, dx=0., wmult=1):
    # Omitting checks for type of dx, wmult
    if image.isinstance(galsim.Image) and not image.getBounds().isDefined():
        self.SBProfile.autoResize(image)
    self.SBProfile.draw(image.view(), dx=dx, wmult=wmult)

As Mike says:

Then the SBProfile version only needs to be defined in terms of an ImageView. But the python version would preserve the automatic resize feature for real Image's, but not ImageView's.

In this design the copy constructor (shallow) and assignment operator (deep) don't have the same semantics, but many on the discussion seemed fine with that -- and indeed some were happy indeed in the change to the previous assignment semantics.

Jim, however, says this:

But I don't see that adding a deep-semantics Image class in addition to the shallow-copy/shallow-assign view class adds that much value in C++ relative to using pointers or a reset method, and in Python it seems like having two classes is essentially unnecessary.

Mike replies:

The value of having both Image and ImageView in both C++ and python is in clarity. You always know who owns the data, and you can very easily declare whether or not you want to duplicate data.

(see usage example above).

E) The first option proposed by Jim which isn't (D), although much is shared with (D) such as making the bounds and pixel scale immutable, and additionally making the assignment operator deep rather than shallow. These two I will simply enumerate as (E) and (F) in what follows.

The problem addressed by Jim's multiple options is that if all we have are the View-like Image classes, we don't have anything we can pass "empty" into a draw routine and have the draw routine resize and allocate it. The first solution is basically the approach advocated in (D) above. The second approach, which we are labelling (E) here, is to say (Jim):

We could add a shallow reset method to the now View-like Image classes, which would essentially do what the assignment operator does now, but with a hopefully less-confusing syntax. This re-raises some of the concerns we wanted to address, but I think it mitigates them quite a bit.

F) Similar to (E), except that instead of giving Images a reset method (handing over to Jim)...

We change the signatures of the draw functions to accept a View-like Image by pointer and return it. When the pointer is null, we allocate create and return new Image, and when it's not null we return a shallow copy after modifying the input in-place. The downside of this is that the draw routines wouldn't be able to return something else (like the flux they currently return), but the advantage is that is translates to a very nice Python syntax, in which we use None instead of an empty Image, and can use the same signature to modify an image in-place or return a new one. To be more precise, here's how a draw function would look in C++:

Image<T> draw(Image<T> * output = NULL, ...) {
    Image<T> result = output ? Image<T>(*output) : Image<T>(getDrawSize());
    ...
    return result;
}

and here's how you'd call it in Python:

image1 = draw(...)  # default argument is None -> NULL
image2 = ImageD(...)
draw(output=image2, ...) # return value is the same as image2, but we can ignore it.

_Summary_
Based on shockingly small number statistics, I sense that the most popular proposal would be (D) or (F), but please correct me if I'm wrong about that. We should have a bit more discussion here, perhaps with interested parties indicating their preferences for (A)-(F) directly on the Issue (no secret ballot, sorry, heh). And of course, If I've misrepresented or missed out something, please suggest corrections or add (G), (H) etc. options.

Once we know what we want to do, we'll decide on who will do it! :)

[rmjarvis]

As you might imagine my preference is D, since I think it has the clearest syntax.

I think much of the confusion with the current implementation comes from having effectively two classes (Image<T> and Image<const T>) trying to implement three concepts:

• an image consisting of (among other things) an array of pixels
• a const view into another array of pixels (e.g. a sub-image or an array owned by pyfits)
• a mutable view into another array of pixels.

The concepts seems to track exactly with the same concepts for matrices that I dealt with for TMV, and choice of shallow/deep copy constructors and assignment operators that I use there have proven to be very intuitive. So I think this would also be true of a similar implementation for Image.

And if we do choose this option, I'd be happy to take the lead on implementing it.

Peace,
Mike

On Apr 25, 2012, at 6:05 PM, Barnaby Rowe wrote:

OK, so here's an attempt at a summary of options that have gained some measure of popular support (using capitalized latin characters due to the mild profusion of other enumerators already on this list!). These seem to me to be the main ones, but please add more if you feel I've missed something that has been discussed:

A) Gary's first "conservative" option:

Jim fixes the resize() issue, otherwise stay the same with the convention that each instance has its own independent labeling of the pixel coordinates, and resizing always disconnects any sharing.

B) Gary's second "conservative" option:

Make the bounds and pixel scale immutable. Very simple to code and use, but might be inefficient as would require a copy of the data array to a new instance when you want to relabel pixel coordinates.

C) Gary's option 3, not discussed very much:

Share bounds and pixel scales, do some extra bookkeeping on subimages or just disallow changing pixel labelling on subimages.

D) Basically Gary's option 4, combined with the first 'flavour' of Jim's proposed solution, which I think is being most strongly advocated by Mike based on his experience with TMV.This seems like it would be in the form of a Image and ImageView classes at the Python level.

As Jim says:

The current Image classes are very close to the View concept Mike brought up. If we remove resize, make the bounds and pixel scale immutable, and additionally make the assignment operator deep rather than shallow, I think they'd be exactly what we'd want a View class to be. This solves the confusion with the assignment and augmented assignment operators that Mike brought up in #114, and it also addresses Gary's concerns with subimages changing coordinate systems. And I don't have any concerns about whether it still works well with NumPy.

Example usage given by Mike would be:

im2 = Image(im1) # Copies the data
im2 += sky_image # Only im2 is affected
im3 = ImageView(im1) # We are just making a view of im1's data
im3 += sky_image # Both im3 and im1 are affected

Mike also points out that if we can separate out the automatic resize functionality from the actual drawing step of the draw() method, then this might be implemented to handle both types easily while letting SBProfile deal only with Views:

def draw(self, image, dx=0., wmult=1):
   # Omitting checks for type of dx, wmult
   if image.isinstance(galsim.Image) and not image.getBounds().isDefined():
       self.SBProfile.autoResize(image)
   self.SBProfile.draw(image.view(), dx=dx, wmult=wmult)

As Mike says:

Then the SBProfile version only needs to be defined in terms of an ImageView. But the python version would preserve the automatic resize feature for real Image's, but not ImageView's.

In this design the copy constructor (shallow) and assignment operator (deep) don't have the same semantics, but many on the discussion seemed fine with that -- and indeed some were happy indeed in the change to the previous assignment semantics.

Jim, however, says this:

But I don't see that adding a deep-semantics Image class in addition to the shallow-copy/shallow-assign view class adds that much value in C++ relative to using pointers or a reset method, and in Python it seems like having two classes is essentially unnecessary.

Mike replies:

The value of having both Image and ImageView in both C++ and python is in clarity. You always know who owns the data, and you can very easily declare whether or not you want to duplicate data.

(see usage example above).

E) The first option proposed by Jim which isn't (D), although much is shared with (D) such as making the bounds and pixel scale immutable, and additionally making the assignment operator deep rather than shallow. These two I will simply enumerate as (E) and (F) in what follows.

The problem addressed by Jim's multiple options is that if all we have are the View-like Image classes, we don't have anything we can pass "empty" into a draw routine and have the draw routine resize and allocate it. The first solution is basically the approach advocated in (D) above. The second approach, which we are labelling (E) here, is to say (Jim):

We could add a shallow reset method to the now View-like Image classes, which would essentially do what the assignment operator does now, but with a hopefully less-confusing syntax. This re-raises some of the concerns we wanted to address, but I think it mitigates them quite a bit.

F) Similar to (E), except that instead of giving Images a reset method (handing over to Jim)...

We change the signatures of the draw functions to accept a View-like Image by pointer and return it. When the pointer is null, we allocate create and return new Image, and when it's not null we return a shallow copy after modifying the input in-place. The downside of this is that the draw routines wouldn't be able to return something else (like the flux they currently return), but the advantage is that is translates to a very nice Python syntax, in which we use None instead of an empty Image, and can use the same signature to modify an image in-place or return a new one. To be more precise, here's how a draw function would look in C++:

Image<T> draw(Image<T> * output = NULL, ...) {
   Image<T> result = output ? Image<T>(*output) : Image<T>(getDrawSize());
   ...
   return result;
}

and here's how you'd call it in Python:

image1 = draw(...)  # default argument is None -> NULL
image2 = ImageD(...)
draw(output=image2, ...) # return value is the same as image2, but we can ignore it.

_Summary_
Based on shockingly small number statistics, I sense that the most popular proposal would be (D) or (F), but please correct me if I'm wrong about that. We should have a bit more discussion here, perhaps with interested parties indicating their preferences for (A)-(F) directly on the Issue (no secret ballot, sorry, heh). And of course, If I've misrepresented or missed out something, please suggest corrections or add (G), (H) etc. options.

Once we know what we want to do, we'll decide on who will do it! :)

---

Reply to this email directly or view it on GitHub:
#117 (comment)

[TallJimbo]

Also unpredictably, my personal preference is for F. I think Mike's assessment is correct about there being three different concepts in C++, from the conceptual standpoint of C++'s ability to distinguish between shared ownership, transfer of ownership, and copying at a basic level (not to mention constness). But in Python, where ownership of the pixels is reference-counted (as I think it must be in any implementation that works with NumPy), and the language doesn't distinguish between pass-by-value and pass-by-reference (Python passes everything by reference), having different classes for different categories of ownership is actually more confusing.

I think the following must be true for an version of option D that works with NumPy and/or behaves "naturally" in Python:

• Both images and views can modify the pixel values.
• When an image goes out of scope, its pixel values must not be deallocated until all views to it also go out of scope.
• When an image is resized or otherwise reallocated, we cannot invalidate any existing views; instead they'd have to keep pointing to the pixels of the original image. While we could refuse to reallocate the image when any views into it exist, that's a restriction I think NumPy users would find arbitrary and unhelpful (note that NumPy arrays are essentially view classes that can never be reallocated).
• I think that means there are only two distinctions between an image and a view in Python:
  • An image can resize or otherwise reallocate itself (without affecting any views).
  • When you call the image constructor with another image or view, it makes a deep copy. When you call the view constructor with another image or view, it makes a shallow copy.

So from the Python perspective, separating images and views is just one interface for supporting deep copies and resize operations. It's the most intuitive and clearest API for those operations in C++, but I don't think it's nearly as intuitive in Python, where shallow copies are the norm, copy constructors aren't any more standard than having a .copy() method, and the lack of resize operations on NumPy arrays hasn't stopped them from being useful.

All that said, I think D is a better solution for a pure C++ design, and it may be the best one for a hybrid C++/Python design in which we expect to do a lot of full-image manipulation (not just pixel-setting) in C++. Most of my arguments are predicated on the assumption that our C++ code will rarely need to allocate, resize, or deep-copy images, and when it does, option F (or E) would be sufficient.

[rmjarvis]

On Apr 25, 2012, at 9:38 PM, Jim Bosch wrote:

I think the following must be true for an version of option D that works with NumPy and/or behaves "naturally" in Python:

• Both images and views can modify the pixel values.
  Yes.
• When an image goes out of scope, its pixel values must not be deallocated until all views to it also go out of scope.
  Can you explain why an image would go out of scope, and yet we would still have views that we care about?
• When an image is resized or otherwise reallocated, we cannot invalidate any existing views; instead they'd have to keep pointing to the pixels of the original image. While we could refuse to reallocate the image when any views into it exist, that's a restriction I think NumPy users would find arbitrary and unhelpful (note that NumPy arrays are essentially view classes that can never be reallocated).
  Again, why should a user expect old views to be valid when they have resized an image? What's the use case here?
• I think that means there are only two distinctions between an image and a view in Python:
  • An image can resize or otherwise reallocate itself (without affecting any views).
  • When you call the image constructor with another image or view, it makes a deep copy. When you call the view constructor with another image or view, it makes a shallow copy.
    Also, only an Image can be constructed as a blank image given either some Bounds or (ncol,nrow).

Mike

[TallJimbo]

On 04/25/2012 10:48 PM, Mike Jarvis wrote:

On Apr 25, 2012, at 9:38 PM, Jim Bosch wrote:

• When an image goes out of scope, its pixel values must not be deallocated until all views to it also go out of scope.
  Can you explain why an image would go out of scope, and yet we would still have views that we care about?
• When an image is resized or otherwise reallocated, we cannot invalidate any existing views; instead they'd have to keep pointing to the pixels of the original image. While we could refuse to reallocate the image when any views into it exist, that's a restriction I think NumPy users would find arbitrary and unhelpful (note that NumPy arrays are essentially view classes that can never be reallocated).
  Again, why should a user expect old views to be valid when they have resized an image? What's the use case here?

In both cases, it's not so much a use case as a safety guarantee. All
variables in Python are reference-counted, and it's not supposed to be
possible to create things like dangling references or invalidated
iterators from Python, even by using wrapped C/C++ in an incorrect way.
Even fatally incorrect usage should always result in a Python exception.

That makes my concern sound like it's just an implementation question -
how do we invalidate a view such that it raises an exception instead of
segfaulting? That's part it - in particular, there's no way to
invalidate a NumPy array, so if we have allow NumPy views into our
images we can't have all views be invalidated by images going out of
scope - all we could do is invalidate our own views, leaving any pure
NumPy ones that may exist.

But I also think invalidating any views - even by raising exceptions -
would be considered surprising behavior by many Python users familiar
with NumPy. There's no distinction between a NumPy array that was
constructed on its own and one that's a view to another, and it's not
uncommon to keep a view array around longer than the array it was
created from.

• I think that means there are only two distinctions between an image and a view in Python:
  • An image can resize or otherwise reallocate itself (without affecting any views).
  • When you call the image constructor with another image or view, it makes a deep copy. When you call the view constructor with another image or view, it makes a shallow copy.
    Also, only an Image can be constructed as a blank image given either some Bounds or (ncol,nrow).

If there isn't any reference counting, then it makes sense to only
provide a variety of constructors for the image class, because it needs
to take ownership of the memory it allocates and ensure it's deleted.

But if you have reference counted ownership, any of the objects can do
the deleting, so there's no reason the view can't have those
constructors too. I'm willing to be flexible on this point if we'd
prefer to only put those constructors on an Image class - creating an
Image and then creating a view from it isn't a terribly penalty, and if
we buy fully into option D it makes more conceptual sense - but I do
think it's an arbitrary restriction.

Jim

[rmjarvis]

On Apr 25, 2012, at 11:35 PM, Jim Bosch wrote:

But I also think invalidating any views - even by raising exceptions -
would be considered surprising behavior by many Python users familiar
with NumPy. There's no distinction between a NumPy array that was
constructed on its own and one that's a view to another, and it's not
uncommon to keep a view array around longer than the array it was
created from.

Sorry to be a broken record about this, but I still don't see the use case where this would become an issue. Why would someone want to get a numpy array view of an Image and then expect it to be valid after the original Image goes out of scope?

Mike

[gbernstein]

On Apr 26, 2012, at 8:02 AM, Mike Jarvis wrote:

Sorry to be a broken record about this, but I still don't see the use case where this would become an issue. Why would someone want to get a numpy array view of an Image and then expect it to be valid after the original Image goes out of scope?

I don't know the technical python issues here so I won't intervene on that account.

But I suspect, Mike, the issue is not whether someone would want to do this. The issue is whether they will do it, intentionally or not, and then if they will get a clear indication that they have made a mistake (instead of silently incorrect answers) and how to fix it. And also of course whether the code allows them to make the mistake in the first place. In other words, are we making a system that is robust to typical python coding practice and some level of malpractice by our future users who will not be as well-informed about the nature of the code as the coding group is.

[rmjarvis]

Agreed. But I still want to know the kind of use case that we need to either allow or guard against. Maybe it's due to my C++ mindset, but I just don't have a good idea of what kind of code Jim is concerned about here.

[TallJimbo]

Here's a slightly contrived example; I can't think of why we'd want to do this in the context of simulating galaxies, but I think it's a reasonable thing to be able to do with an image class and I certainly wouldn't want to rule it out.

Say we wanted to load a FITS image from disk, but we really only care about a subset of the image:

bounds = BoundsI(...)
image = galsim.fits.read("file.fits").subimage(bounds)

There are other ways to accomplish the same thing that don't have view invalidation problems, of course, and some of them may be more efficient. But this is a not-unreasonable way to do it, and we don't want to force people writing Python to start worrying about ownership or memory management when deciding how to do something like this.

[rmandelb]

Merged in pull request (issue #128), closing.

[rmjarvis]

Sorry I was late getting on this morning. I hear that you gave my talk for me. Thanks. :)

Mike