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

Make ConstrainedPolyModel actually support initFromWCS

    Details

      Description

      While sorting out the best approach for getting jointcal to ingest a SkyWcs, we discovered that the ConstrainedPolyModel was ignoring the initFromWCS constructor boolean flag. Looking at the history, it appears it was never used: the chip transfo was always being initialized with the input WCS.

      This is an easy fix, and I'll also make it a configurable, so we can test how larger datasets behave when it is False; the testdata all appears to be fine (initial chi2 is different, but it quickly settles down to the same value).

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Since you helped find it, you get to review it. It's short.

            Show
            Parejkoj John Parejko added a comment - Since you helped find it, you get to review it. It's short.
            Hide
            rowen Russell Owen added a comment -

            Looks good to me

            Show
            rowen Russell Owen added a comment - Looks good to me
            Hide
            astier Pierre Astier added a comment -

            I cannot remember why I put this option originally, but it never proved to be useful so far. I tend to think that given the model, not initializing the chip transfos using WCS's looks awkward. The fact that it works without that on small-scale tests does not make it safe for large fits. I don't see now any reason for not initializing the Chip mappings using WCSs. Is there any?
            One important point in the initialization is to define the range over which the Mapping is going to operate, encapsulated into the first argument of the constructor. This remaps the CCD coordinates on [-1,1] and very significantly improves the condition number of the linear system, which is critical for large problems.
            Whatever happens, this remapping of the coordinates should remain, I think.

            Show
            astier Pierre Astier added a comment - I cannot remember why I put this option originally, but it never proved to be useful so far. I tend to think that given the model, not initializing the chip transfos using WCS's looks awkward. The fact that it works without that on small-scale tests does not make it safe for large fits. I don't see now any reason for not initializing the Chip mappings using WCSs. Is there any? One important point in the initialization is to define the range over which the Mapping is going to operate, encapsulated into the first argument of the constructor. This remaps the CCD coordinates on [-1,1] and very significantly improves the condition number of the linear system, which is critical for large problems. Whatever happens, this remapping of the coordinates should remain, I think.
            Hide
            Parejkoj John Parejko added a comment -

            The input WCS (usually higher-order SIP) may not map reliably onto the initial chip WCS (currently 2nd order polynomial, eventually 3d (x,y,z) affine). For the SkyWcs conversion (replacing the guts of afw::wcs with AST), it would be easier for us if we didn't need to know the parameters of the input WCS.

            I agree that the remapping should remain, and that should be unaffected by this change: GtransfoLin shiftAndNormalize = normalizeCoordinatesTransfo(frame); is included whether the initial WCS is used or not.

            Show
            Parejkoj John Parejko added a comment - The input WCS (usually higher-order SIP) may not map reliably onto the initial chip WCS (currently 2nd order polynomial, eventually 3d (x,y,z) affine). For the SkyWcs conversion (replacing the guts of afw::wcs with AST), it would be easier for us if we didn't need to know the parameters of the input WCS. I agree that the remapping should remain, and that should be unaffected by this change: GtransfoLin shiftAndNormalize = normalizeCoordinatesTransfo(frame); is included whether the initial WCS is used or not.
            Hide
            Parejkoj John Parejko added a comment -

            Pierre Astier Now that this is configurable, we can actually test it on other datasets and see if it matters. We might talk about it when we meet on Tuesday.

            Thanks for the review, Russell Owen. Merged and done.

            Show
            Parejkoj John Parejko added a comment - Pierre Astier Now that this is configurable, we can actually test it on other datasets and see if it matters. We might talk about it when we meet on Tuesday. Thanks for the review, Russell Owen . Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Russell Owen
                Watchers:
                Dominique Boutigny, John Parejko, John Swinbank, Pierre Astier, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel