Aside from several trivial doc fixes, the review mostly discussed the question of whether the efforts at backwards compatibility were worthwhile:
Simon Krughoff:
I notice a lot of work goes into making sure that version 0 tables work. I realize that backward compatibility is ideal, but we have frequently broken backward compatibility. Is it necessary to go to these lengths to make version 0 tables work, or am I missing the point of version 0 tables?
Jim Bosch:
If this was just about the transition period before meas_base can do everything the old meas_algorithms could do, I'd be more inclined to agree. But this is also necessary to allow outputs produced by older versions of the pipeline to be read by newer versions of the pipeline. Eventually I hope we can remove the backwards compatibility layer for that too, but I imagine that could be a long time after we've made the transition, and there's no longer any old outputs we care about being able to read.
Robert Lupton:
Couldn't we do this at the butler level? Read the old inputs and rename the columns?
Jim Bosch:
We could, I suppose. That seems harder, because it won't always be a one-to-one mapping between new fields and old fields. It would also make it less useful for the immediate (but more temporary) purpose of easing the transition to meas_base.
If we were to translate the old catalogs into new ones, I think it'd work best as a script you'd run to convert an entire data repository, at which point we could force the user to provide additional information (e.g. via config) to clear up any ambiguities in the mapping. But I'd rather not worry about that until the transition is over. It may have been a mistake to work this hard on this kind of backwards compatibility from the beginning, but I definitely don't want to switch to a different strategy right now.
Robert Lupton:
I agree that it's too late.
Note that my on-the-fly solution would allow old and new versions of the code to both read the files.
Jim Bosch:
True, and that'd definitely be desirable. It'd be difficult to implement at the Butler level, though, in the sense that all the Butler does with these things is pass a filename to the afw FITS-reading code. We'd either need to read it in and then write it back out to a temporary file using low-level FITS manipulations (using e.g. PyFITS), before passing it to afw, or read it into an old-style afw::table object in memory before doing the mapping to a new-style afw::table object (in which case we'd need at least some of this version code to support that temporary).
Simon, could you take a look at this? It's rather large, but not huge: it's a rewrite of how the slot mechanism in SourceTable works, to make use of the new alias mechanism added just before.
I've squashed commits in order to make them a bit cleaner, and you might find it easier to review this one commit at a time rather than looking at the full diff at once (essentially none of the commits overlap, but this way the git logs will give you an idea of what each changeset was for).
However, you probably want to start with the docstring for the SlotDefinition class in afw/table/slots.h, as that gives a pretty good overview of how this is all supposed to work.
Changes are limited to afw, and all on branch u/jbosch/
DM-419:afw:u/jbosch/DM-419 % git diff --stat master...u/jbosch/DM-419
include/lsst/afw/table/Schema.h | 4 +-
include/lsst/afw/table/Source.h.m4 | 396 +++++++++---------------------------
include/lsst/afw/table/slots.h | 287 ++++++++++++++++++++++++++
python/lsst/afw/table/Match.i | 23 ---
python/lsst/afw/table/Source.i | 27 +--
src/table/AliasMap.cc | 2 +-
src/table/Source.cc | 237 ++-------------------
src/table/io/FitsReader.cc | 27 +++
src/table/slots.cc | 197 ++++++++++++++++++
tests/data/slotsVersion0.fits | 1 +
tests/testSourceTable.py | 58 ++++--
tests/ticketDM-433.py | 34 ++--
12 files changed, 678 insertions(+), 615 deletions(-)