wiki:ChangeImageAPIs
Last modified 11 years ago Last modified on 09/02/2008 09:09:12 PM

Proposed changes to Image, Mask, and MaskedImage for DC3

This page is superseded by wiki:RhlImageAPIs.


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

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

1/ The current APIs for the Image classes expose the vw API. I believe that this is a mistake as it makes our API more complex than necessary, and prevents us from moving away from vw should it prove desireable (n.b. I am not aware of any current need to replace vw, unless some of the following proposals are infeasible in the vw context). In other words, make the image classes adaptors for vw::ImageView

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/ Provide adaptors (?afw::math::BBox2i) for vw::BBox2i (and associated classes) [Note: Russell proposes simply importing the vw classes into our namespace, but I'd rather not have any vw include files included into our code]

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

2/ Make the proper cosmetic changes to boost::shared_ptr typedefs, such as changing

typedef boost::shared_ptr<Image<ImagePixelT> > ImagePtrT;

to

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

and add

typedef boost::shared_ptr<const Image<ImagePixelT> > const_Ptr;

N.b. This added typedef is needed in all LSST classes.

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

3/ Make

Ptr getSubImage(const BBox2i imageRegion) const;

and

Ptr getSubMask(const BBox2i maskRegion) const;

return a shallow copy.

At the same time handle const properly, e.g.

Ptr getSubImage(const BBox2i imageRegion);

and

const_Ptr getSubImage(const BBox2i imageRegion) const;

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

4/ Add APIs that are needed but currently provided by call downs to the vw code (n.b. these may be simply forwarded to vw)

a/ For example, we'll need an API to make deep copies of the Image classes.

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])

but simply exposing the row pointers would be simpler. If we go this way, we should explicitly declare that all LSST images are stored in C (i.e. column index increases fastest) order. The underlying implementation could still be strided arrays, of course.

c/ We need to think about MaskedImage accessors. The normal use case is that we need access to all three components.

d) We need a good way to do absolute addressing of MaskedImage pixel data.

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

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 assigment 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.

Comment by ktl on Tue 22 Apr 2008 02:27:08 PM CDT

For 1c: No VW include files should be explicitly listed in code outside the image classes, but not having them included, even indirectly, by any non-AFW code may be more difficult. At a minimum, it is likely to require pimpl-style programming.

For 2: const_Ptr should probably be ConstPtr (my preference) or PtrConst to fit with LSST naming conventions.

For 5: Augmented assignment operators returning the LHS is not only for chaining these but also to allow for standard assignment of the result. This is still uncommon.

Comment by rowen on Mon 28 Apr 2008 02:54:09 PM CDT

For 1: I feel that (c) argues for offering a pixel accessor for MaskedImage (as we have now). To my mind, this in turn suggests using a pixel accessor for Image and Mask, but one that is optimized to be as fast as row pointer access. For efficiency I believe we must either offer an optimized version of pixel_accessor for data stored in C order or else (simpler but less flexible) always store data in C order. I prefer the latter. Once we have pixel_accessor I think we should use it instead of row pointers.

A long-shot alternative is to use direct indexing of the image as our preferred way to access data. This solves a-d but I call it a long-shot because I don't believe it can be as efficient as pixel-accessors or row pointer access.

I also would like to have an STL-compatible iterator (we could just offer thin layer on the iterator that vw already has) for those cases when one does not need to know the pixel coordinates. But I suggest we eliminate PixelProcessingFunction? and use std::for_each instead.

Unfortunately we end up with 3 or 4 different ways to access pixel data: direct indexing, pixel-accessors, an STL iterator and possibly row pointers.

For 5: I agree that we can return None. But I would prefer we also spend the $1k and fix the SWIG problem since it may block us from doing useful things in the future.

Comment by rowen on Fri 02 May 2008 04:52:42 PM CDT

At present readFits is a method -- it overwrites the current image data (resizing the image if necessary). I feel it would be more natural to have these be constructors for Mask, Image, MaskedImage and Exposure.

Comment by rowen on Fri 02 May 2008 05:00:51 PM CDT

There is one subject your proposal does not address that I would very much like to see resolved for DC3: more graceful handling of FITS header metadata.

At present Image and Mask objects can read data from FITS files. While doing this the entire contents of the FITS header is read into a metadata field in the object.

Issues with the current design include:

  • Some of the metadata is redundant, e.g. image size and type.
  • The metadata is confusingly distributed for MaskedImage (image, mask and variance) and Exposure (all those plus WCS and likely other metadata for DC3). I think we basically only pay attention to the metadata in the image Image.
  • When creating an Exposure the WCS information is read from the FITS metadata, but then the two exist side by side. The FITS metadata will be wrong if the WCS object is updated.

(Note that the current design makes sense from a historical perspective: Image and Mask came long before Exposure and we needed some way to read FITS files.)

I propose:

1) Eliminate FITS metadata from Image and Mask. Only allow metadata for Exposure.

This fits the basic model that Exposure is an image plus metadata (well, a MaskedImage plus metadata). Note that Image and Mask may still be read from FITS files if desired, but one does lose FITS header metadata in the process.

2) Avoid redundancy by discarding FITS metadata that we use.

In other words, for all FITS metadata that we know how to use in some LSST-standard way, deal with it properly and discard the original FITS metadata. For example:

  • Read the WCS FITS size to set the object, then discard the FITS metadata.
  • Read the WCS FITS cards to create a Wcs object, then discard the FITS metadata.
  • Discard pixel type information: once the data is read into an Image we know what type it is.
  • Any FITS headers that can be transformed to standard LSST metadata in some way are handled that way.

Any remaining FITS metadata gets saved to a field that we keep around for completeness but should NOT use in our pipelines. Call it "leftoverMetadata" or something like that.

Comment by Tim Axelrod on Mon 05 May 2008 05:31:16 PM CDT

I think the image metadata issue deserves some more thought before we adopt a design. Here's are some thoughts:

What do we want? I think there are two big things:

  1. Metadata should maintain its association with an image throughout the

lifecycle of the image. This lifecycle typically involves some or all of the following steps:

  1. The image is created by some source like a camera, along with metadata.
  1. The image is persisted to some external file format such as FITS.
  1. The image is read in from the external format, thereby constructing some object(s) in a software system.
  1. The image is manipulated within the software system, making use of some subset of the associated metadata values, possibly changing those values, and potentially generating additional metadata that will be used in subsequent processing.
  1. After some sequence of processing steps, the image is again persisted to an external format, possibly a different one that it began with.
  1. The value of a metadata item should be unambiguous at all times. It

should never be possible to get one value with an explicit reference to the metadata, and another from an object attribute that logically represents the same quantity.

In looking toward a viable design, it's fair to note that when image metadata is stored, it naturally seems to end up with a table (keyword

  • value) representation. This is true not only for FITS, but also for

AVM and XMP, both of which we will probably have to deal with in our interface to the public.

To me, this sounds very close to the problem that object-relational mappers (ORM) are intended to solve. So my suggestion is that we look at the capabilities of the ones that are out there and at a minimum use them for design ideas. What we are trying to do is pretty simple compared to what some of the ORM solutions are designed for. And particularly given that we are already using ORM for persistence (even though we decided to ditch the one we were using), it might not be too much of a stretch to use that technology here as well.

Tim

Add comment