Discussion about unifying CoaddPsf and CoaddBoundedField: DM-834

Data Management
#1

DM-834 is a request to unify CoaddPsf and CoaddBoundedField. CoaddPsf uses ExposureTable, whereas CoaddBoundedField (CBF) contains an std::vector of CoaddBoundedFieldElement, and there is much overlap:

ExposureTable's ExposureRecord contains:

  • apCorrMap, which contains a dict of name: boundedField
  • bbox
  • validPolygon
  • wcs
  • weight (an added field)

plus psf and calib, two fields not needed by CoaddBoundedField.

CoaddBoundedFieldElement contains:

  • boundedField (one per boundedField in an apCorrMap)
  • bbox (as part of boundedField)
  • validPolygon
  • wcs
  • weight

Note: for a given exposure there is one ExposureRecord, but potentially many CoaddBoundedFieldElements, since each apCorrMap may contain many bounded fields.

For unifying these I lean towards having CBF take an ExposureTable that has one field added: a BoundedField. However:

  • I don’t yet know how difficult it is to add a BoundedField to a schema.
  • CBF’s constructor would require a key or name for each added field: weight and boundedField

Also, CBF would ignore 3 fields in the table, but this does not worry me as they could be empty pointers.

Another option is to add a new kind of table that contains the common subset of ExposureTable and CoaddBoundedFieldElement:

  • bbox
  • validPolygon
  • wcs
  • weight

If we go that route then users of EposureTable need to add psf, calib and apCorrMap and users of CoaddBoundedFieldElement would have to add a single boundedField. Note that users of ExposureTable rely on convenience functions to access the extra elements, so this would require at least one new table class.

An option that would require no changes, but which I strongly dislike, is to construct a CBF
using an ExposureTable with just the weight field added and encode the BoundedField as a single entry in apCorrMap. This seems like an abuse of apCorrMap and would require extracting the contained boundedField from the apCorrMap in each record every time the CBF was evaluated at a point.

As an aside, ExposureTable is little more than an unpacked ExposureInfo. I wonder if it would be cleaner for the table to store the ExposureInfo itself. That would eliminate the convenience methods (we just need one to get the ExposureInfo, then call its methods to get contained data) at the expense of one added level of lookup. This would be even nicer once ExposureInfo gains a bbox. However, I this would be a much more disruptive change (though ExposureTable is not used in very many places).

#2

Well, this is worse than I’d remembered. I don’t particularly like any of the options here, and the only one I think I’d consider better than the status quo is a variant on the one you disliked.

I don’t think just adding a BoundedField to ExposureRecord is a good idea; ExposureRecord, because there isn’t a single BoundedField associated with an exposure that it would represent in any context outside CoaddBoundedField.

Defining a minimal record class that both CoaddPsf and CoaddBoundedField implementations could use as a base class is a good idea in general, but adding new record classes requires so much boilerplate I’m not a fan of that route either.

The possibility I was thinking of would be to actually keep the full ApCorrMap with all BoundedFields present, and share the full internal ExposureCatalog across the different CoaddBoundedField objects in the coadd’s ApCorrMap. That would lock CoaddBoundedField in as only being useful for aperture corrections (which, to be fair, may be all it ever would be anyhow), and it’d require an extra layer of indirection for peristence, since the framework won’t currently allow a Catalog to be shared between persisted objects directly:

class CoaddBoundedField : public table::io::Persistable {
public:
...
private:

    struct Impl;

    std::string _apCorrName;
    shared_ptr<Impl> _impl;
};

struct CoaddBoundedField::Impl : public table::io::Persistable {
    table::ExposureCatalog catalog;
};

I’m ambivalent about whether that’s all a good idea, and I’m leaning towards saying it’s not a good use of our time right now. Ultimately, I want to make it possible to add an object field like a BoundedField dynamically to a Schema rather than requiring that they be hard-coded in via inheritance. That should eliminate the need for different Record subclasses altogether and provide a much more elegant solution to the problem.