refactor SBProfile classes to be immutable and/or use shared_ptr

[TallJimbo]

A bug Mike just fixed in SBAdd reminded me of my nervousness about the raw pointers used in SBProfile. Now that we've got boost::shared_ptr available to us, I think we could make the code a lot safer and easier to maintain by using that in many cases.

Before doing that, however, I think we should take a look and see if we can make the SBProfile classes (or perhaps just most of them) immutable. They have very few mutators currently; the main ones we'd have to worry about are SBProfile::setFlux, SBAdd::add, and SBConvolve::add. We'd have to replace these with functions that created a new, appropriately modified SBProfile. If we can make everything immutable, we can also get rid of the duplicate() member functions, and we'll be able to get even more out of using shared_ptr.

Even if it turns out that immutability is not the right design choice, I think we should still start using shared_ptr in a number of places. This would also provide a nice solution for #76, as we could use shared_ptr to ensure the lifetime of the interpolant is at least that of the SBPixel that relies on it.

[rmandelb]

One further issue to consider here:
My impression is that we'd been avoiding making too many changes to the C++ SBProfile code (so that if Gary is continuing to separately develop SBProfile, we can benefit from that development being directly importable here, modulo superficial details like namespaces -- and likewise, he can benefit from bug-fixes that we make here, such as the one Mike just found in SBAdd). Such a change would clearly be a deviation from that informal policy.

I'm not saying we shouldn't do what you say, but it's another factor to weigh in the calculation.

[TallJimbo]

I agree. I definitely wouldn't want to jump into this without having Gary fully behind it.

[rmjarvis]

It looks like the main non-const method that would require some thought is setFlux. Every SBProfile class has it, and it does seem to be used in non-trivial ways (e.g. by SBParse). On the other hand, it probably wouldn't be too bad to have a new class that represents a scaled SBProfile (SBMult or SBScaled perhaps) that just keeps a scaling factor and a pointer to another SBProfile object.

The other non-const methods are add (SBConvolve and SBAdd), setRd and setFWHM (both SBMoffat). But none of these seem to be used in ways that couldn't be incorporated into a constructor. At least in the code in the GalSim repository, that is.

There is also an op= to check into, but that didn't grep so well, so I'm not sure how much that gets used.

@gbernstein Gary, do you use the non-const methods very much in other code that you have? Would you be averse to this idea of making all SBProfile objects immutable? I agree with Jim that this can make the code a lot cleaner and avoid the duplications that happen now in things like SBAdd.

[gbernstein]

It may make for some painful constructors for SBProfiles that have many degrees of freedom, and maybe some loss of speed in calculations with objects whose flux has been rescaled with the new adapter class. In particular the SBPixel (which now has some other name?) would need to be passed all of its pixelized data plus all of the information about interpolation in the constructor.

But the concept is very do-able, if anyone wants to invest the time to re-coding in exchange for having later use be less prone to user error.

On Apr 16, 2012, at 11:57 AM, rmjarvis wrote:

It looks like the main non-const method that would require some thought is setFlux. Every SBProfile class has it, and it does seem to be used in non-trivial ways (e.g. by SBParse). On the other hand, it probably wouldn't be too bad to have a new class that represents a scaled SBProfile (SBMult or SBScaled perhaps) that just keeps a scaling factor and a pointer to another SBProfile object.

The other non-const methods are add (SBConvolve and SBAdd), setRd and setFWHM (both SBMoffat). But none of these seem to be used in ways that couldn't be incorporated into a constructor. At least in the code in the GalSim repository, that is.

There is also an op= to check into, but that didn't grep so well, so I'm not sure how much that gets used.

@gbernstein Gary, do you use the non-const methods very much in other code that you have? Would you be averse to this idea of making all SBProfile objects immutable? I agree with Jim that this can make the code a lot cleaner and avoid the duplications that happen now in things like SBAdd.

---

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

[rmandelb]

Pinging @barnabytprowe @rmjarvis @TallJimbo @gbernstein @pmelchior @joezuntz @joergdietrich @reikonakajima

This issue + the connected #76 (SBProfile immutability, shared_ptr) is probably worth resolving before we work on the next milestone. (For the record, the other issues that I'd love to see done during this time are #134 on shearing conventions; #4, which Barney is working on now; and #141, which Mike is working on now. If this turns out to be too ambitious then we can reprioritize and/or make some of these part of the next milestone itself.)

First question:
Is there general agreement that this is a good idea? My own take is that it is worthwhile. While I have taken advantage of functions that change SBProfiles (mostly setFlux but sometimes =) I would consider the loss of those functions a small price to pay given what we'd gain from the change.

If so, then the work involved would be:

• going through SBProfile.h and SBInterpolatedImage.h to make a complete list of functions that change SBProfiles, and that need to be removed or reworked
• decide how to deal with those (some of them are obvious, others less so)
• use more shared_ptr in pysrc/ -- I'm not familiar enough with it to know how/where that needs to be done.
• rework galsim/base.py to not rely on C++ functions like duplicate that we'll be removing.

[gbernstein]

A potentially relevant comment: the photon shooting (pull request coming today or tomorrow) will attach a lot of precomputed information some kinds of SBProfiles, namely some lookup-table kind of things. The time to compute these will probably be equal to the time it takes to shoot many thousands of photons. So it will be wasteful to recalculate them by creating new instances of class, let's say, SBMoffat, every time you make an object. It will be better to make copies of a single parent object to avoid the recomputations. Also the code will be set up to do these calculations only when needed.

I don't think this will interfere with making objects conceptually immutable - but it would be good for others to think about how this would be implemented. And also how the immutability will interact with our desire to have multithreaded code. Is the pointer sharing going to be thread-safe?

[rmjarvis]

We don't need to lose setFlux at the python level. It would be a wrapper around an SBScale class just like applyRotation is really a wrapper around an SBRotate class. So as long as we're ok with losing setFlux at the C++ level, this would not be hard to do at all. I'm fine with taking it on.

[rmandelb]

Yes, I agree, we can have setFlux in python, just not in C++ (I've used it in both).

[rmjarvis]

I don't think the precomputed stuff should be a problem. I suspect photon shooting will be merged in before this issue, so I'm fine with including making that work correctly as part of this issue.

[rmandelb]

If there's a pull request for photon shooting this week then it should be in before this issue gets dealt with.

Gary's question about the impact on multi-threaded code is a good one that we should think about...

[TallJimbo]

boost::shared_ptr is thread safe to the extent that there's a lock on the reference count. The user is still responsible for making sure only one thread is modifying the pointed-at object at once. But if the classes are truly immutable then we don't have to worry about that either. Of course, one could imagine a class with an immutable API but mutable cache data members it constructs on first use (rather that on construction). That would not be "truly immutable", but if this comes up I think we worry about making it thread safe if and when we decide to use threads, and we could do that by putting a lock around those mutable data members.

The other big changes that would be required aside from setFlux would be the classes that are based on lists of other SBProfiles, like SBAdd and SBConvolve. In one sense, those would get a lot nicer because they wouldn't have to copy their elements, but in many cases it's useful to append to those in-place. We might be able to retain that by making a free function or static member function that appends to something given a shared_ptr, and only does a copy if the pointer is non-unique:

class SBAdd {
public:
    ...
    static boost::shared_ptr<SBAdd> void append(
        boost::shared_ptr<SBAdd> target,
        boost::shared_ptr<SBProfile> const & element
    ) {
        if (!target.unique()) target = boost::make_shared<SBAdd>(target); // invoke copy constructor
        target->_list.push_back(element);
        return target;
    }
}

This is a slightly ugly interface in C++ because we can't make it a regular member function, but it would be perfectly natural in Python.

[joergdietrich]

I have no opinions on C++ code.

[rmjarvis]

I don't think it's a hardship to always construct Add and Convolve with the full list. Especially at the python level, it is easy to append to a list, and then when ready, make the container object with the final list.

In C++, we can retain the add function, but make it private and used only by the class's constructor.

[gbernstein]

Jim raises a good point, which is that it will no longer be possible to build SBAdd or SBConvolve one term at a time using the append() method without generating a new object every time. I would hope that the existence of the shared pointers could be completely private in the C++ code whereas Jim's code fragment returns them. We'll see how the implementation goes.

Jim, when you say there is a lock on the reference count of shared_ptr, what multithreading library (libraries) does this lock work with?

[TallJimbo]

Actually, it looks like I may have lied about the lock, but the thread safety guarantee is the same as what I described. I'm not really sure how it works, but here's a link to the documentation:

http://www.boost.org/doc/libs/1_48_0/libs/smart_ptr/shared_ptr.htm#ThreadSafety

[rmandelb]

Okay. I'm getting the sense that this is probably worthwhile but will take more work than I initially expected to handle things like the photon-shooting, the classes like Add and Convolve, etc. (none insurmountable but still, there will be a lot of systematic changes to the code that need to be thought through and tested carefully).

I had initially proposed this for during our cleanup period because I thought it is better done in a quieter period rather than when we're in the midst of lots of coding for the milestone. But given the amount of work, I think it's a bit much to expect someone to do this in just one week. If there is someone who is willing to do this in the next ~2 weeks I suspect that will be okay... that's getting into the start of the next milestone period, but it's still early enough that people can merge in the new code to their branches relatively early on without things being too chaotic. Or, we could think about whether we want to adjust the next milestone period to allow for this to all take place first.

Does someone particularly want to take this on?
I'm about to go volunteer myself for the shear conventions issue, not this one (I don't think this one is the best fit for my time availability or my coding skills, errr, non-skills.)

[rmjarvis]

I don't mind.

[rmandelb]

Thank you - but, given that it does seem like a fair amount of quite technical work, perhaps we should have a discussion on the phone call (Tues) as to whether people really want this to happen now, or whether there might be some value in waiting?

On May 18, 2012, at 10:41 AM, Mike Jarvis wrote:

I don't mind.

---

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

---

Rachel Mandelbaum
http://www.astro.princeton.edu/~rmandelb
rmandelb@astro.princeton.edu

[rmjarvis]

I think I'll start working on this next. I've gone through the SBProfile class in detail, and here are the public member functions that are currently not const, so we at least need to think about them:

• setFlux: Everything has this. My solution to deal with it is to treat it just like the current shear, distort, rotate, and shift methods. i.e. return an SBDistort of the original profile that has the new flux information. I'll have to add a little bit to SBDistort to make this work, but not much.
• operator=: Everything has this. Probably just make it invalid, since I don't think we need it for anything.
• add: SBAdd and SBConvolve have this. I'll keep this, but as a private member function that is only called by the constructors. We'll get rid of this at the python level too. I think we should always be able to call the constructor that takes the full list, so I don't think this is a hardship in practice.
• setFWHM and setRd in SBMoffat. We don't need these.
• setPixel, setWeights, setXInterpolant, setKInterpolant in SBInterpolatedImage. We don't currently use any of these, but obviously they are there for a reason. In fact, they are the only way to set the _wts variable. @gbernstein Gary, what is the use case for these? I assume it is mostly for when you have more than 1 image. Can we get all of the functionality for this by adding a constructor that takes a list of images?

We also need to think about anything that is marked mutable.

• _sampler in SBAiry. Only updated by checkSampler.
• fluxes, xFluxes, yFluxes, vk in SBInterpolatedImage. Only updated by checkReady.
• xsum in SBInterpolatedImage. Only updated by checkXsum.
• ksum in SBInterpolatedImage. Only updated by checkKsum.
• positiveFlux, negativeFlux, pt in SBInterpolatedImage. Only updated by checkReadyToShoot.
• ready, xSumValid, kSumValid, readyToShoot in SBInterpolatedImage. They are updated by the current non-const methods that we are going to get rid of. And by their corresponding "check" method.

So no problem for any of them. They are all the kind of thing that could in principal be setup in the constructor, but we defer until later for possible efficiency reasons. (Especially the shoot stuff, since we often won't need it.) And they are all updated in only one location, which makes it easy to put mutex guards around them if we every decide that we want these to be thread safe to make sure that only one thread updates them. In other words, none of them are the kind of mutable variables that cause problems for an officially "immutable" object.

Anyone have any objections to any of this? I'll talk about implementation details next, but wanted to get all of this vetted first.

[rmjarvis]

OK, implementation details. Here is what I was thinking of doing:

• Make SBProfile's concrete objects using the pimpl idiom, rather than a virtual base class.
• The SBProfileImpl class would be a local protected class that is a virtual base class.
• The SBProfile's only constructor would be a copy constructor.
• Each other derived class would define its own Impl class derived from SBProfileImpl to define the various virtual functions. Basically shifting everything in the current classes except the constructor down a level into the impl
• The constructor for the derived classes would simply make the pimpl object with the correct Impl class.

So the user interface would basically be equivalent to the way you use things in python. Everywhere we currently use SBProfile*, we could now just use an SBProfile object directly. The copies would all be fast, shallow copies. And the objects can be used in containers like list<SBProfile>.

This would also allow us to shift pretty much all of the implementation details from the .h file into the .cpp file. Which means that SBProfile.cpp would get even larger. So I think we should split it up into more separate files corresponding to the different derived classes. However, as far as I can tell, there is no git analog to svn copy, so I think this means that the history gets clobbered for each of the new files I would create from the current SBProfile.cpp. I guess this is ok, but a bit unfortunate.

Comments on this plan?

[TallJimbo]

When you change how setFlux works, would it be possible to make all the non-modifier SBProfiles have unit flux?

[rmjarvis]

When you change how setFlux works, would it be possible to make all the non-modifier SBProfiles have unit flux?

I suppose, but is this desirable? It would mean that to have a Gaussian with a particular flux, you would specify this in two steps:

SBGaussian gauss(sigma);
SBProfile gauss2 = gauss.setFlux(flux);

rather than just

SBGaussian gauss(flux,sigma);

We could do it that way, but I guess I don't see any advantage to it.

[TallJimbo]

I think I like the virtual pimpl approach, but it's not clear to me how the outer classes would work. Would there be a full hierarchy of nonpolymorphic classes that mirrors the polymorphic impl hierarchy? Or just one outer SBProfile class and various ways to construct it with different impl classes?

Here are the advantages of the impl approach that I can think of (relative to just making the public classes polymorphic and putting them in shared_ptrs):

• It allows us to add in-place mutators that actually do copy-on-write if we want to later (i.e. re-enable add for SBAdd and SBConvolve in such a way that we modify them in-place if and only if the impl pointer is unique), in a way that's transparent to the user.
• We don't have to worry about methods that would need to construct a shared_ptr from this, because these can be implemented in the outer class. And we'd need a lot of these (e.g. distort needs to pass a pointer to this to the constructor of the new SBDistort object). The alternative (without pimpl) would be to use boost::enable_shared_from_this, which isn't bad either, but probably isn't quite as clean.
• We can easily ensure all the impl classes are always allocated and owned by shared_ptrs.
• Less implementation code in headers.
• Signatures that take an SBProfile are somewhat cleaner (const SBProfile & a vs. boost::shared_ptr<SBProfile> a), and we can usually use the prettier (and more Python-like) '.' syntax rather than '->' to access their methods.

There are some disadvantages, however:

• Exposing immutable, noncopyable objects directly to Python within boost::shared_ptr is slightly cleaner than "regular" copyable objects, because it avoids the mismatch between constness and copy semantics between the two languages. But I can't think of anything concrete this would affect in a negative way, unless we did have both const and non-const methods on the outer SBProfile class(es).
• If we only have a single outer class, we'd lose any derived-class-specific interfaces (e.g. radius getters). That might be a price worth paying, as I think it's more in line with the overall "feel" of SBProfile as a single interface with lots of implementations. More importantly, I think we're using those derived-class-specific interfaces very rarely, if ever.
• If we have a hierarchy of outer classes, we need to think about what happens in conversions between them. Do we need to implement our own versions of static and dynamic cast operators, for instance, to cast an outer SBProfile by value to a derived class? That might be the case if functions that can return different types of SBProfile have to return a base class object, but other code wants to access the derived-class-specific interface. Those operators are already defined for shared_ptrs to polymorphic classes, so they wouldn't be hard to implement, but that starts to feel like we're losing more than we gain from putting a wrapper around shared_ptr. And Boost.Python will automatically create a derived class wrapper from a base class shared_ptr return (using RTTI). It would be very tricky to do that with our own nonpolymorphic outer classes.
• If we have a hierarchy of outer classes, there's some extra boilerplate involved in having two classes for each SBProfile.

I'm not convinced we'd get a lot of value from pimpl unless we do want to use copy-on-write instead of pure immutability, and I think Mike's made a pretty good case that we can get by with pure immutability. But I also think a pimpl with a single outer class is also a very clean design that I wouldn't have any problem with, if we can live with removing the derived-class-specific behavior (which I don't think we're using at present). A pimpl approach with a nonpolymorphic outer hierarchy worries me a little more; it might be fine, but we should think about return types and casting before pushing forward with it.

[TallJimbo]

On git: there is some support for "move detection". If two files are similar enough, git will notice that one is a copy/rename of the other. So it might be worth doing the refactor into separate files separately from any editing of the files to make that work better. But I'm not sure how well that works when splitting one file up into several anyhow, and I don't think it's a major issue. The history will still be there, it just won't quite be associated with the new code.

[rmjarvis]

My idea was that there is only one pimpl, which is in the SBProfile class.

Derived classes are polymorphic with SBProfile, but they are very simple. The constructor makes the (SBProfile) pimpl with the correct derived Impl class that actually does all the work. If there are any class-specific things, they are also defined, since they know they can static_cast the pimpl to their particular Impl class and use it how they want.

SBProfiles are generally passed around by value, since the shared_ptr is internal.

I can't think of any reason we would need a conversion between outer classes, so I don't think your concern about the dynamic casting issue is going to bite us at all. We will have copy constructors for the various derived classes that work just like the base class. So I was planning on having shear() et all return an SBDistort by value rather than an SBProfile. This could be true for anything else that returns an SBProfile object. If you are really making one of the derived classes, you can return that instead. Then if people want to use it as the derived class, they just do so. If they don't care what it is, they can receive it as an SBProfile, which would also work.

The only downside I see is your first point that we will effectively have a second shared_ptr from python on top of the one that we have in C++ rather than being able to join them up into the same thing. But I can't really see any reason why that would be a problem.

[rmjarvis]

To give a bit more concreteness to my proposal, I started the conversion so people can see what it would look like. I converted SBProfile, SBAdd and SBDistort to the pimpl idiom. You can peruse the code here.

Jim, maybe we could do a design review for this to discuss the different issues that you brought up. Maybe Monday or Tuesday? I'll set up a doodle poll if you think it would be worthwhile.

[rmjarvis]

I think I've finished the last important item that could really benefit from shared_ptr. The latest one is the Random classes. I rearranged this a bit in a way that I think is pretty nice. The old syntax still works:

ud = UniformDeviate(lseed)
gd = GaussianDeviate(ud,mean,sigma)

However, if you don't actually need ud, now you can instead simply do:

gd = GaussianDeviate(lseed,mean,sigma)

There is also a version that will automatically create the RNG using the time:

gd = GaussianDeviate(mean,sigma)

Furthermore, the constructor that used to take a UniformDeviate as the first argument now can take any class in the hierarchy of random deviates (anything that derives from the new BaseDeviate class).

The way this works is that the base class now holds the boost:mt19937 object in a shared_ptr. So it can be passed around among various derived classes, all of which will use the same underlying random number generator.

I also added reset methods that sever the connection of a particular deviate from any other deviate that had been sharing the rng with it, and then later reconnect it if you want. I don't know if we would ever need this, but it seemed like a neat feature that was easy to add, so I did.

[rmjarvis]

Also, I mentioned this on the telecon, but for Gary's benefit, I've also finished implementing option 2 that we talked about for SBInterpolatedImage. There is a new class MultiImageHelper that encapsulates all the information about the multiple images. Then you can create an SBInterpolatedImage from that and a vector of weights. So I think that gets all the functionality that we would need for this.

There are a couple more places in the code that use bare pointers. SBParse, ProbabilityTree and Pset. Should I tackle those too? I don't think any of them are that important to change. (i.e. no issues about custodian and ward or similar) And I don't think there are any memory bugs in them, so it would be purely for cosmetic reasons I think.

[rmandelb]

Nice!

On Jun 5, 2012, at 7:01 PM, Mike Jarvis wrote:

I think I've finished the last important item that could really benefit from shared_ptr. The latest one is the Random classes. I rearranged this a bit in a way that I think is pretty nice. The old syntax still works:

ud = UniformDeviate(lseed)
gd = GaussianDeviate(ud,mean,sigma)

However, if you don't actually need ud, now you can instead simply do:

gd = GaussianDeviate(lseed,mean,sigma)

There is also a version that will automatically create the RNG using the time:

gd = GaussianDeviate(mean,sigma)

Furthermore, the constructor that used to take a UniformDeviate as the first argument now can take any class in the hierarchy of random deviates (anything that derives from the new BaseDeviate) class.

The way this works is that the base class now holds the boost:mt19937 object in a shared_ptr. So it can be passed around among various derived classes, all of which will use the same underlying random number generator.

I also added reset methods that sever the connection of a particular deviate from any other deviate that had been sharing the rng with it, and then later reconnect it. I don't know if we would ever need this, but it seemed like a neat feature that was easy to add, so I did.

---

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

---

Rachel Mandelbaum
http://www.astro.princeton.edu/~rmandelb
rmandelb@astro.princeton.edu

[rmjarvis]

Merged into master.

[barnabytprowe]

Great, thanks Mike...
On Jun 11, 2012, at 4:31 PM, Mike Jarvis wrote:

Merged into master.

---

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

Barnaby Rowe
Postdoctoral Research Associate

Jet Propulsion Laboratory
California Institute of Technology
MS 169-237
4800 Oak Grove Drive
Pasadena CA 91109
United States of America

Department of Physics & Astronomy
University College London
Gower Street
London WC1E 6BT
United Kingdom

[rmandelb]

Fantastic, thanks!

On Jun 11, 2012, at 7:31 PM, Mike Jarvis wrote:

Merged into master.

---

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

---

Rachel Mandelbaum
http://www.astro.princeton.edu/~rmandelb
rmandelb@astro.princeton.edu