# Adapt SIP fitter for writing approximations to general Wcss

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
4
• Sprint:
DRP S18-1, DRP S18-2, DRP S18-3, DRP S18-4, DRP S18-5, DRP S18-6, DRP F18-1, DRP F18-2, DRP F18-3
• Team:
Data Release Production

## Description

Relocate and refactor the new TAN SIP fitter from meas_astrom to afw to allow it to be used to write TAN SIP approximations to more general WCSs.

## Activity

Hide
Jim Bosch added a comment -

Russell Owen, the code is now up and running, and the high-level interface you'll be working with (afw::geom::SipApproximation) is documented. You're welcome to rebase on this branch and start integrating it into SkyWcs persistence.

I still have some documentation to do for the lower-level components before I put this out for review. I'd ideally also like to refactor some older code in shapelet and ChebyshevBoundedField to make use of those components (there's a lot of duplication now), but I may just create new tickets for some of that.

Show
Jim Bosch added a comment - Russell Owen , the code is now up and running, and the high-level interface you'll be working with (afw::geom::SipApproximation) is documented. You're welcome to rebase on this branch and start integrating it into SkyWcs persistence. I still have some documentation to do for the lower-level components before I put this out for review. I'd ideally also like to refactor some older code in shapelet and ChebyshevBoundedField to make use of those components (there's a lot of duplication now), but I may just create new tickets for some of that.
Hide
Jim Bosch added a comment - - edited

Russell Owen, I gather you made not need this as urgently as you once thought, but it's now complete as far as I'm concerned, and it's time to get it reviewed. As the main consumer, I'm afraid you're also the natural reviewer.

There's a fair amount of new code here (especially the second commit, which adds the whole afw/math/polynomials directory). For that commit, it may be helpful to start with the namespace documentation for lsst::afw::math::polynomials (in afw/math/polynomials.h) to get the big picture up front.

I've also created a number of tickets (linked to this one) to do the second half of the integration and refactoring this library represents (basically, I gathered a lot of code from a lot of places and cleaned it up and made it work together, without going back and changing the places I took it from). None of those is totally trivial, so I don't think it makes sense to do any of them here.

Show
Jim Bosch added a comment - - edited Russell Owen , I gather you made not need this as urgently as you once thought, but it's now complete as far as I'm concerned, and it's time to get it reviewed. As the main consumer, I'm afraid you're also the natural reviewer. There's a fair amount of new code here (especially the second commit, which adds the whole afw/math/polynomials directory). For that commit, it may be helpful to start with the namespace documentation for lsst::afw::math::polynomials (in afw/math/polynomials.h) to get the big picture up front. I've also created a number of tickets (linked to this one) to do the second half of the integration and refactoring this library represents (basically, I gathered a lot of code from a lot of places and cleaned it up and made it work together, without going back and changing the places I took it from). None of those is totally trivial, so I don't think it makes sense to do any of them here.
Hide
Russell Owen added a comment - - edited

Jim Bosch and I discussed this at the DM-SE JTM (March 2018) and have agreed to attempt to get permission for and implement the following plan:

• Put the new polynomial classes into the geom package, as well the core classes that are presently in lsst.afw.geom, including Angle, Point, Extent, Box, LinearTransform and AffineTransform. We will add alisas to lsst.afw.geom so that no existing code is broken by this change. Note that most of geom has been made obsolete by sphgeom and our code uses very little of it, so we hope to remove most or all of the current contents at the same time.
• Merge this ticket.
• As time permits: modify our existing code that computes polynomials and Chebyshev polynomials to use the new implementation in geom. This should be a very simple change.

skypix is the main package using geom and we no longer use skypix so I filed RFC-458 "Retire skypix".
Jim Bosch will also file an RFC for moving the core lsst.afw.geom classes into geom (while leaving aliases for backwards compatibility).

Show
Russell Owen added a comment - - edited Jim Bosch and I discussed this at the DM-SE JTM (March 2018) and have agreed to attempt to get permission for and implement the following plan: Put the new polynomial classes into the geom package, as well the core classes that are presently in lsst.afw.geom , including Angle , Point , Extent , Box , LinearTransform and AffineTransform . We will add alisas to lsst.afw.geom so that no existing code is broken by this change. Note that most of geom has been made obsolete by sphgeom and our code uses very little of it, so we hope to remove most or all of the current contents at the same time. Merge this ticket. As time permits: modify our existing code that computes polynomials and Chebyshev polynomials to use the new implementation in geom . This should be a very simple change. skypix is the main package using geom and we no longer use skypix so I filed RFC-458 "Retire skypix". Jim Bosch will also file an RFC for moving the core lsst.afw.geom classes into geom (while leaving aliases for backwards compatibility).
Hide
Jim Bosch added a comment -

Russell Owen, I've finally gotten around to moving the new polynomials library to lsst.geom and updating afw accordingly.  I believe this is ready for review again.  All previous review comments (which were only on the code that is now in geom) have been addressed as well.

Show
Jim Bosch added a comment - Russell Owen , I've finally gotten around to moving the new polynomials library to lsst.geom and updating afw accordingly.  I believe this is ready for review again.  All previous review comments (which were only on the code that is now in geom) have been addressed as well.
Hide
Russell Owen added a comment -

This looks very useful. Some comments on github.

Show
Russell Owen added a comment - This looks very useful. Some comments on github.
Hide
Jim Bosch added a comment -

Merged to master, after probably setting at least personal records for number of sprints a ticket has been part of and total time spent in "review complete".

Show
Jim Bosch added a comment - Merged to master, after probably setting at least personal records for number of sprints a ticket has been part of and total time spent in "review complete".

## People

• Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Russell Owen
Watchers:
Jim Bosch, Russell Owen