wiki:RhlImageAPIs
Last modified 11 years ago Last modified on 09/05/2008 02:20:46 PM

Proposed changes to Image, Mask, and MaskedImage for DC3

This page supersedes wiki:ChangeImageAPIs.


Let us call Image, Mask, and MaskedImage the, "Image classes"

0/ At present, the lowest level image classes are Image and Mask, and they are a mixture of pure image processing classes and metadata. For example, what does it mean to add two Images with different WCSs? Or identical WCS but different metadata?

With this in view, TSA originally specified that Image would include a pointer to a pure image processing type. As discussed below, I think that the choice of a boost::shared_ptr<vw::ImageView> was a mistake, but the basic idea is certainly sound.

This proposal is therefore concerned with the definition of the pure pixel classes. We need another class (or set of classes) that include e.g. WCS and other metadata and can be mapped to FITS files (although this is NOT required --- merely a useful way to think about their meaning). Various candidates have been proposed (Image; Exposure; MaskedImage). As explained above, I don't think that Image is appropriate. A MaskedImage (with variance and mask) is not a good model of a simple astronomical image (although it may well match most LSST data), and therefore neither is the rather more complicated Exposure. We therefore suggest putting another class into the hierarchy called (provisionally) DecoratedImage that contains an Image::Ptr.

Note that this is similar to the current Image, with vw::ImageView playing the role of the Image::Ptr. The advantages of the current proposal are:

  • The properties of Image are clearly defined
  • We are not tied to vw's implementation details (e.g. their iterator model)
  • We are not tied to vw at all, if we decide that we don't like the way that they are going, or the level of support

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

1/ Modify the current APIs for the Image classes to remove the vw API:

a/ Remove all public inheritance from vw::PixelIterator from MaskedImage (n.b. it is not acceptable to switch to protected!)

b/ Remove all public/protected data members of type vw::ImageView from the Image classes. This will involve removing accessors to vw::ImageView<> and vw::PixelChannelType and reworking the pixel accessors as we can no longer use vw::ImageView<>::pixel_accessor

c/ Remove classes such as vw::Bbox2i; these may be replaced by e.g.

   template<typename T> class Point<T> { ... };
   typedef Point<int> PointI;
   typedef Point<double> PointD;
   typedef std::pair<PointI, PointI> Bbox;

No user code should explicitly include implementation header files (e.g. vw or boost::gil). This doesn't preclude their being indirectly included by e.g. Image.h

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

2/ (This was the

        typedef boost::shared_ptr<Image<ImagePixelT> > Ptr;
        typedef boost::shared_ptr<const Image<ImagePixelT> > ConstPtr;

proposal, and it passed the CCB. However, it is not yet fully implemented. )

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

3/ Replace

        Ptr getSubImage(const BBox2i imageRegion) const;

by a copy constructor:

        Image(const Image& src, const Bbox& bbox, const bool deep=false);

where the copy is shallow unless otherwise specified.

Assignments will still be shallow, but the operator <<= will be overloaded to provide a deep (i.e. pixel) copy. E.g.

    Image<float> img(7, 4);
    img = 100;

    Image<float> subImg = Image<float>(img, Bbox(PointI(1, 1), 5, 2));
    Image<float> devil = Image<float>(5, 2);

    devil = 666;
    subImg <<= devil;

will have the effect of setting the centre of img to 666 (as would subImg = 666;)

I withdraw proposals dealing with const variants until we have more experience.

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

4/ Add APIs as needed to replace those currently implemented by unpacking the vw pointers.

a/ Some of these will simply require forwarding the calls to the underlying layer.

b/ Pixel accessors are a little harder. I believe that we need to provide simple access to the rows of the images; it would be possible to augment the vw accessors in two ways:

1/ Provide a != operator

2/ Allow for relative referencing (e.g. cursor[-1])

However, I'd prefer to switch to the pixel-access approach of the boost::gil library (http://stlab.adobe.com/gil/html/giltutorial.html and http://stlab.adobe.com/gil/html/gildesignguide.html).

Specifically, provide iterator, reverse_iterator, c_iterator, r_iterator, and cr_locator. The first two are STL iterators. The next two are iterators for columns and rows (the former are inefficient for data stored in C's standard row-order. I propose that we standardise in this data layout). The last "iterator" is {const_}?cr_locator and may be used to traverse an image in arbitrary ways --- but is designed for efficient iterators to move a set of iterators through an image:

void y_gradient(const Image& src, const Image& dst) {
    Image::const_cr_locator src_loc = src.cr_at(0, 1);
    Image::const_cr_locator::cached_location_t above = src_loc.cache_location(0,  1);
    Image::const_cr_locator::cached_location_t below = src_loc.cache_location(0, -1);

    for (int r = 1; r < src.getRows() - 1; ++r) {
        for (Image::c_iterator dst_it =
                 dst.row_begin(r); dst_it != dst.row_end(r); ++dst_it, ++src_loc.c()) {
            *dst_it = (src_loc[above] - src_loc[below])/2;
        }
        
        src_loc += std::make_pair(-src.getCols(), 1);
    }
}

(parenthetically, the inner loop could have been written as

*dst_it = (src_loc(0, 1) - src_loc(0, -1))/2;

if direct coordinate-based indexing is prefered; but it's likely to be slower. src(row, col) is also supported, but slow.)

c/ This approach generalises to MaskedImage, but this isn't discussed in this proposal.

d/ As Russell notes, with the std::for_each there is little need for PixelProcessingFunction and we should drop them. As r_iterator is more efficient, we can provide a for_each_pixel generic function that allows more efficient access than the general iterator for data with gaps between the rows.

e/ The const iterators and locators should be provided:

const_iterator
const_reverse_iterator
const_c_iterator
const_r_iterator
const_cr_locator

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

5/ The standard definition augmented assignment operators (e.g +=) operators returns the lhs; this allows me to write a += b += c;

This is uncommon (it takes a moment to see that this means a += b + c; b += c;) and of limited value.

I propose that we require that all augmented assignment operators return void. This is advantageous in the swig world as the ownership of objects is a little murky. If we do not do this, we need to fix swig --- I can get a contract to do it, but it'd cost c. $1000.

Update: I might like to withdraw this proposal too. If we don't use shared_pointers, operator op=() works just fine (although it's a little scary as swig doesn't really understand that the lhs is the same object as is returned). However, if we use a void not an Image& return type, the swigged python's doesn't work, but it can be fixed in the .i file. I lean to keeping the void and using my swig macros. Comments?

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

6/ Russell would like to replace readFits() by a constructor.

This is a reasonable, but I don't think that we should bundle it with this proposal.

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

7/ I have removed the Metadata discussion from this proposal as it's become its own separate proposal.

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Comment by ktl on Wed 03 Sep 2008 03:37:03 AM CDT

3/: Should the copy constructor use an Image::bbox from 1/c?

4/b and 4/d: Should the naming of iterators and types match STL standards, Boost standards, or LSST standards?

Comment by rhl on Thu 04 Sep 2008 09:18:05 PM CDT

  • Note that I added a section on const iterators, and modified the op= proposal

In answer to KT:

  • In 3 I changed the subimage copy constructor to use image::Bbox.
  • In 4/[bd], the names are taken from boost::gil and are modelled on the STL; the STL compliant ones are {const_}?{reverse_}?iterator. The {const_}?[cr]_iterators iterate in row or column, and the {const_}?cr_locator doesn't quite behave like a standard iterator. I propose that we indeed keep these names.

Add comment