Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-936

Replace getXXXKey for slots with returning functorKeys similar to existing compound Keys

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:

      Description

      Make any changes in the Functor Key capabilities necessary to support getXXXKey, getXXXErrKey, and getXXXFlagKey for the 3 different types of slots.

      Change these routines in Source.h.m4 to return FunctorKeys for both version 0 and version 1 tables. Then fixup any compilation breaks on the C++ size which this causes. Remove the version 1 specific accessors.

      I am assigning this to Jim to confirm that the first part is done, then he can assign the remainder to Perry either by reassigning, or as a subtask.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            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.

            Show
            jbosch Jim Bosch added a comment - 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.
            Hide
            pgee Perry Gee added a comment -

            I think that the only controversial thing in this checkin is that I removed the ability to save or load slots which are defined with non-alike names. This was originally permitted by the constructor which allowed a slot to be defined with 3 (possibly compound) keys for meas, err, and flag. I couldn't find anywhere is the code where this was used and then the slot was permanently saved to a table, so I believe it to be safe.

            Show
            pgee Perry Gee added a comment - I think that the only controversial thing in this checkin is that I removed the ability to save or load slots which are defined with non-alike names. This was originally permitted by the constructor which allowed a slot to be defined with 3 (possibly compound) keys for meas, err, and flag. I couldn't find anywhere is the code where this was used and then the slot was permanently saved to a table, so I believe it to be safe.
            Hide
            jbosch Jim Bosch added a comment -

            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.

            Show
            jbosch Jim Bosch added a comment - 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.

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              pgee Perry Gee
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: