Recreating some comments from Perry and I that were lost in a failed JIRA transition. Perry is in quotes.
In order to get the meas_algorithms unit tests to pass, the order
of aggregates.i in tableLib.i (in afw) was moved up to in front of
Source.i. I thought the change in order would not affect aggregates.i,
but Jim should review.
Review complete.
afw, Source.h.m4: do we need the addCentroidMeasFields() and
addShapeMeasFields() convenience functions? I suspect that they won't
be used anywhere unless we try to convert meas_algorithms-style
algorithms to use version 1 tables (which I don't want to do; I'd rather
convert these algorithms to the meas_base interface).
afw, Source.h.m4: please revert the whitespace change near
makeFitsWriter (newline should go after this method, not before it).
afw, Source.h.m4: why rename .e.g hasCentroid() to hasCentroidSlot()?
(if both of these are moderately new, I don't care, but if hasCentroid()
has been around a while we shouldn't change it without an RFC). And if
we don't change this, then we need no changes for this issue in
meas_astrom or meas_base.
afw, Source.h.m4: after this change, I thought we'd be able to just
remove the old slot data members (_slotFlux, _slotCentroid, _slotShape),
and then only look at tableVersion in the methods that define the slots.
Does that not work?
ip_diffim, dipoleMeasurement.py: please revert the whitespace change
after docstring of SourceFlagChecker.
The convenience functions are used for version 1 tables. There
are several places in places like meas_astrom, ip_diffim, and ap where
compound fields for Shapes or Centroids are added on the fly. I just
want to make it less verbose to add them.
I changed the name of hasCentroid() because the macro to create the
function did not seem to work without something before the (). I am
willing to go back to hasCentroid() if you can tell me what punctuation
I need. But I don't think it matters since I just added this method,
and I actually like hasCentroidSlot better, as it is more
descriptive.
I guess it would be OK to remove the old _slots. We just can't
reinstate the old macros, since some of the slots return Keys, and some
FunctorKeys.
I did not see any places where the convenience functions were used
on this ticket. Are you sure they aren't all in meas_algorithms-style
algorithms that need to be more fully converted to meas_base
plugins?
I'm fine with leaving hasCentroidSlot().
If it is okay to remove the old _slots, please do.
The use of the convenience functions is in code which I have
already changed in various places, but pulled out when we decided to end
the Sprint without testing it (for example, it is needed in ip_diffim
and ap, and could be used in other places - e.g., the distortedCentroid
in meas_astrom.
I think these functions will be needed in some form in the end, and I
just want to avoid having to change the code back and forth to meet
Sprint endings.
It will take me an hour or more to fix the _slots. I hadn't really
realized that we were done needing them. I'm so used to being in this
intermediate state of supporting both versions.
The FunctorKeys don't quite have everything you need, but I can easily describe what needs to be added so Perry can do it:
In CovarianceMatrixKey, we need to add a bool data member that controls whether the saved values along the matrix diagonal are variances (version 0) or sigmas (version 1). We'll also need to be able to set that in the constructor. With that in place, we should be able to use all of the FunctorKeys to return the version 0 slots.
I'm hoping you can implement this by just storing the version 0 slot keys within FunctorKeys, so we don't have to create them on-the-fly (in the case of CovarianceMatrix, at least, constructing them will be a bit of a pain so it'd be nice to only do it when the slot is defined). But I haven't looked in detail to see if that will be possible.