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

add task to meas_astrom to fit an aribtrary WCS with a TAN-SIP WCS

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_astrom
    • Labels:
      None
    • Story Points:
      1.5
    • Team:
      Alert Production

      Description

      Sometime in the past, Russell Owen wrote a method to take an arbitrary WCS and approximate it as a TanWcs (our implementation of FITS' TAN-SIP WCS formalism). That method is currently just a utility function in the unit test testFitTanSipWcsHighOrder.py. This issue will promote that method to a full-fledge task in meas_astrom.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            Oops. I see that you changed afw to add wcsNearlyEqualOverBBox. Sorry I missed that the first time. My initial reaction is that it's not really necessary and DM-3347 might suffice with less clutter. However, others in RFC-28 did call for predicates so I may be in the minority in that opinion. My discomfort with predicates in situations where something is evaluated at many points (e.g. comparing WCSs) is as follows:

            • A predicate in production code should probably short-circuit rather than testing at every point, whereas a unit test assert should test every point and report the worst behavior. Thus it is not trivial to implement the latter in terms of the former and the predicate should probably not produce the same detailed information as the assert.
            • In this case your code would run faster with a short-circuiting predicate; I don't know if it's worth adding that.

            In any case, I am against adding a predicate unless it short circuits (or can be made to do so). Being able to short circuit or not sounds a bit messy, as is being able to return information about the failure. Maybe one public function with a simple, clean interface can do it all, but if not, another option is to write an underlying implementation that has a messier interface and call that from the public predicate and public assert.

            Finally...do you really need to call this assert/predicate at all? I thought the fitter returned some measure of success. Maybe it would suffice to use that?

            Show
            rowen Russell Owen added a comment - - edited Oops. I see that you changed afw to add wcsNearlyEqualOverBBox. Sorry I missed that the first time. My initial reaction is that it's not really necessary and DM-3347 might suffice with less clutter. However, others in RFC-28 did call for predicates so I may be in the minority in that opinion. My discomfort with predicates in situations where something is evaluated at many points (e.g. comparing WCSs) is as follows: A predicate in production code should probably short-circuit rather than testing at every point, whereas a unit test assert should test every point and report the worst behavior. Thus it is not trivial to implement the latter in terms of the former and the predicate should probably not produce the same detailed information as the assert. In this case your code would run faster with a short-circuiting predicate; I don't know if it's worth adding that. In any case, I am against adding a predicate unless it short circuits (or can be made to do so). Being able to short circuit or not sounds a bit messy, as is being able to return information about the failure. Maybe one public function with a simple, clean interface can do it all, but if not, another option is to write an underlying implementation that has a messier interface and call that from the public predicate and public assert. Finally...do you really need to call this assert/predicate at all? I thought the fitter returned some measure of success. Maybe it would suffice to use that?
            Hide
            danielsf Scott Daniel added a comment -

            Looking at include/lsst/meas/astrom/CreateWcsWithSip.h, it does not appear that the getNewWcs method returns a measure of its success. It just returns the new WCS.

            I will un-do my changes to afw in favor of waiting for DM-3347 and use your MockTest suggestion.

            Show
            danielsf Scott Daniel added a comment - Looking at include/lsst/meas/astrom/CreateWcsWithSip.h, it does not appear that the getNewWcs method returns a measure of its success. It just returns the new WCS. I will un-do my changes to afw in favor of waiting for DM-3347 and use your MockTest suggestion.
            Hide
            rowen Russell Owen added a comment -

            The object returned by makeCreateWcsWithSip is a CreateWcsWithSip. It is true that its getNewWcs() method just returns just a WCS, but it also has other methods, including getScatterInPixels() and getScatterOnSky(), which used together might do the job. These are implemented in C++ and so are likely to be faster than calling that assert function. On the other hand, those methods can only compare at the points that were fit (since that object has no other information), so cannot detect overfitting. You are an expert on fitting, so I'll leave it to you to decide how best to test the result.

            Show
            rowen Russell Owen added a comment - The object returned by makeCreateWcsWithSip is a CreateWcsWithSip. It is true that its getNewWcs() method just returns just a WCS, but it also has other methods, including getScatterInPixels() and getScatterOnSky(), which used together might do the job. These are implemented in C++ and so are likely to be faster than calling that assert function. On the other hand, those methods can only compare at the points that were fit (since that object has no other information), so cannot detect overfitting. You are an expert on fitting, so I'll leave it to you to decide how best to test the result.
            Hide
            rowen Russell Owen added a comment - - edited

            DM-3347 adds a wcsNearlyEqualOverBBox, a predicate that uses short-circuit evaluation, in case you want it. However, I expect review and merge of that ticket to take a few days.

            Show
            rowen Russell Owen added a comment - - edited DM-3347 adds a wcsNearlyEqualOverBBox, a predicate that uses short-circuit evaluation, in case you want it. However, I expect review and merge of that ticket to take a few days.
            Hide
            danielsf Scott Daniel added a comment -

            I've already merged this ticket. I figure, if we want to, we can update the approximateWcs in the future.

            Show
            danielsf Scott Daniel added a comment - I've already merged this ticket. I figure, if we want to, we can update the approximateWcs in the future.

              People

              • Assignee:
                danielsf Scott Daniel
                Reporter:
                danielsf Scott Daniel
                Reviewers:
                Russell Owen
                Watchers:
                Paul Price, Russell Owen, Scott Daniel
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel