DM-12764: Overhaul SkyWcs #294

r-owen · 2017-11-30T17:27:27Z

No description provided.

The error string was mis-formatted

TallJimbo

Looks good overall! I have made some comments that I'd like to at least discuss before merging, but nothing that I think will be too difficult to address.

My only big-picture comment is that it strikes me that we could - and probably should - split this into a pure-abstract base class and a concrete subclass with a Transform implementation, without too much difficulty. The fact that the original Wcs base class included a wcslib backend ended up being a bit of a pain point for us, and I'd like to avoid having the same problem develop here.
The current SkyWcs interface has relatively few methods that actually depend on or expose Transform/AST (these would of course go in the concrete subclass's interface only), and I think the ABC interface could be what virtually all code that uses (rather than creates) WCSs would see. Code that does create WCSs would of course use the concrete subclass directly.

TallJimbo · 2017-11-30T17:57:34Z

include/lsst/afw/cameraGeom/Orientation.h

@@ -96,7 +96,7 @@ class Orientation {
     *
     * @returns lsst::afw::geom::Transform from focal plane to pixel coordinates
     */
-    geom::Transform<geom::Point2Endpoint, geom::Point2Endpoint> makeFpPixelTransform(
+    geom::TransformPoint2ToPoint2 makeFpPixelTransform(


This function (and the other in this file) seem to have been missed in the conversion to shared_ptr returns.

Fair enough. I'll also convert the functions in transformFactory.

TallJimbo · 2017-11-30T19:58:40Z

include/lsst/afw/geom/SkyWcs.h

-@param[in] flipX  Fip x axis (see orientation for details)?
+                        At 0 degrees, +Y is along N and +X is along W/E if flipX false/true
+                        At 90 degrees, +Y is along E and +X is along N/S if flipX false/true
+@param[in] flipX  Fip x axis? See orientation for details.


I assume this is correcting misleading documentation, not reflecting changed behavior?

I had flipX wrong initially -- the sign did not agree with the old Wcs.isFlipped method. Fortunately it's mostly used in unit tests that don't actually care, so fixing it required few changes elsewhere.

TallJimbo · 2017-11-30T20:36:46Z

include/lsst/afw/geom/SkyWcs.h

-All are of class ast::Frame except the sky frame. The domain is listed first, if set:
- ACTUAL_PIXEL0 (optional): actual pixel position using the @ref pixel_position_standards "LSST standard";
-    The 0 is to remind users that this frame is 0-based.
+@anchor skywcs_frames The contained ast::FrameDict must have the following frames (listed by domain);


"must" here seems to conflict with "should only be provided if" below.

Note the (optional) in "ACTUAL_PIXEL0 (optional)" . I see the code is slightly out of date, as the name is "ACTUAL_PIXELS". I'll work on fixing that; presumably a later commit on DM-10765 that needs to be moved back.

TallJimbo · 2017-11-30T20:37:52Z

include/lsst/afw/geom/SkyWcs.h

-    SkyWcs &operator=(SkyWcs const &) = delete;
-    SkyWcs &operator=(SkyWcs &&) = delete;
+    SkyWcs &operator=(SkyWcs const &) = default;
+    SkyWcs &operator=(SkyWcs &&) = default;


This change makes SkyWcs mutable. Was that intentional?

I assume you are referring to move assignment mutating the object? Do we care about full replacement as a form of mutation?

It's the latter: full replacement is definitely a form of mutation, and it breaks one of the most important advantages of immutability: it means that one owner of a shared_ptr (or other reference or pointer) cannot assume that the thing it holds (possibly as internal state) will not change.

Fair enough. I'll revert the change.

TallJimbo · 2017-11-30T20:40:13Z

include/lsst/afw/geom/SkyWcs.h

+     * @param[in] projection  The name of the FITS WCS projection, e.g. "TAN" or "STG"
+     */
+    explicit SkyWcs(Point2D const &crpix, coord::IcrsCoord const &crval, Eigen::Matrix2d const &cdMatrix,
+                    std::string const &projection = "TAN");


Just to satisfy my own curiosity, is the switch from Coord to SphPoint happening on DM-10765, or another later ticket?

I had expected to see it on this one, but I don't actually care how it's staged.

It will be on another ticket. It's a big job and will require some changes to SpherePoint (mostly additions).

TallJimbo · 2017-11-30T21:18:53Z

include/lsst/afw/geom/Transform.h

 template <class FromEndpoint, class ToEndpoint>
 class Transform {
+    friend class SkyWcs;
+


Is this just for access to the protected constructor?

TallJimbo · 2017-11-30T21:20:48Z

include/lsst/afw/geom/Transform.h

@@ -250,17 +259,17 @@ class Transform {
     *
     * @param[out] os  outpu stream to which to serialize this Transform
     */
-    virtual void writeStream(std::ostream & os) const;
+    virtual void writeStream(std::ostream &os) const;


I don't think this needs to be virtual anymore; I don't envision Transform having any subclasses, since its polymorphism at the AST level is pretty hard-coded.

I'll mark Transform as final, remove the virtual empty destructors and all virtual modifiers.

TallJimbo · 2017-11-30T21:22:11Z

include/lsst/afw/geom/Transform.h

+     * Construct a Transform from a shared pointer to a FrameSet
+     *
+     * @note The FrameSet may be modified by normalizing the base and current frame.
+     */
    explicit Transform(std::shared_ptr<ast::FrameSet> &&frameSet);


I haven't been able to think of a reason why it would make sense to pass by rvalue reference here. Requiring the shared_ptr to be an rvalue doesn't actually transfer ownership of the pointee or even guarantee that this constructor will be the sole owner (because the shared_ptr passed could itself be a copy). If establishing sole ownership is important, the argument would have to be unique_ptr. If not, just pass the shared_ptr by value (and std::move it into the data member to save a reference-count increment, if you like).

TallJimbo · 2017-11-30T22:12:02Z

include/lsst/afw/geom/detail/wcsUtils.h

+ * Read a CD matrix from FITS WCS metadata
+ *
+ * @throws pex::exceptions::InvalidParameterError if no CD matrix coefficients found
+ * (missing coefficients are set to 0, as usual, but they cannot all be missing).


Please document angle units.

TallJimbo · 2017-11-30T22:19:12Z

include/lsst/afw/geom/SkyWcs.h

+    /*
+     * Construct a SkyWcs from a shared pointer to an ast::FrameDict
+     */
+    explicit SkyWcs(std::shared_ptr<ast::FrameDict> &&frameDict);


I don't think it makes sense to pass this by rvalue reference (see similar comment on the Transform constructor with the same signature).

TallJimbo · 2017-11-30T23:18:29Z

Also, I should add that I have only looked at the C++ headers. Please let me know if there's anything else you'd like me to look at.

r-owen · 2017-12-04T23:54:22Z

I have made all requested changes except adding a new virtual base class.

This appears as a new pull request #295. Please put all new comments there. (I'm keeping DM-10765 up to date with these changes and it's easier this way).

Regarding a new virtual base class: it's not a bad idea, but I think it's far too late. It would require a huge amount of work:

A new class name and separation of all existing usage of SkyWcs into one of the two classes.
Renaming most of the free functions that produce a SkyWcs to identifying if they produce a SkyWcs, a base Wcs or some future kind of Wcs.
Modification to many of the 25+ packages already updated for DM-10765

The intent of SkyWcs was to be flexible enough to handle all our celestial WCS needs. I hope that if we find we need something else then we will replace the guts with something similarly flexible, rather than subclassing.

TallJimbo · 2017-12-05T19:09:18Z

(continuing to comment here just because it's purely replies)

Regarding a new virtual base class: it's not a bad idea, but I think it's far too late. It would require a huge amount of work:

I am not nearly so pessimistic...

A new class name and separation of all existing usage of SkyWcs into one of the two classes.

I think this is a few days of work, and well worth it. Let's call the new things SkyWcs (ABC) and ComposableSkyWcs, for now; SkyWcs would lose then, getTransform, and getFrameDict. I think it could keep everything else (with as many things as you need pure virtual), and the implementations copied verbatim to ComposableSkyWcs.

Renaming most of the free functions that produce a SkyWcs to identifying if they produce a SkyWcs, a base Wcs or some future kind of Wcs.

All functions that return a new SkyWcs could just return a ComposableSkyWcs. While we could quibble whether that's the right approach in a pure C++ system, it's largely irrelevant in a hybrid Python/C++ system.

Modification to many of the 25+ packages already updated for DM-10765

Any code that could use the old Wcs class should be able to use the base class interface and free functions. The only exception is code that called a Wcs constructor directly (instead of e.g. a makeWcs function). The only thing ComposableSkyWcs can do that SkyWcs can't is be composed, and that's something Wcs couldn't do, so we shouldn't have any code that needs it yet.

The intent of SkyWcs was to be flexible enough to handle all our celestial WCS needs. I hope that if we find we need something else then we will replace the guts with something similarly flexible, rather than subclassing.

Given that we cannot extend AST with new transforms ourselves, I think being able to add some level of extensibility in our own code is essential. An ABC would give us a way to have most of our code use astropy or gWCS objects or transforms from other third-party libraries, and it would also protect the vast majority of our code from needing to change if we did in fact drop AST in the future.

timj · 2017-12-05T19:25:09Z

Given that we cannot extend AST with new transforms ourselves, I think being able to add some level of extensibility in our own code is essential. An ABC would give us a way to have most of our code use astropy or gWCS objects or transforms from other third-party libraries, and it would also protect the vast majority of our code from needing to change if we did in fact drop AST in the future.

What does this mean though? You of course can add new transforms to AST. It's just that those transforms have to be part of AST and not externally tacked on. That's not necessarily a bad thing because you don't really want to be in a situation where you write some extended WCS and then require LSST plug in code to read it.

My plan for gWCS interop is to agree on an AST/gWCS serialization form.

TallJimbo · 2017-12-05T19:43:37Z

You of course can add new transforms to AST. It's just that those transforms have to be part of AST and not externally tacked on.

No, I can't, in practice. And neither can other LSST devs. It has been made quite clear that only David Berry is actually capable of doing so in a finite amount of time, and at the risk of reopening an old discussion, I continue to think that's a serious problem we need to mitigate however we can.

That's not necessarily a bad thing because you don't really want to be in a situation where you write some extended WCS and then require LSST plug in code to read it.

That's a problem for things we persist as part of the finished pipeline and give to science users, but it's not a problem for an R&D period in which we're trying to determine the models we'll use before we invest the time/money in getting them upstreamed to AST. Admittedly, that may not even be enough flexibility since those models would not be composable, but I still think some flexibility is better than none.

My plan for gWCS interop is to agree on an AST/gWCS serialization form.

I completely agree that this is the ideal scenario, but I don't want to pull of our eggs in that basket.

TallJimbo · 2017-12-05T19:52:52Z

You of course can add new transforms to AST. It's just that those transforms have to be part of AST and not externally tacked on.

No, I can't, in practice. And neither can other LSST devs. It has been made quite clear that only David Berry is actually capable of doing so in a finite amount of time, and at the risk of reopening an old discussion, I continue to think that's a serious problem we need to mitigate however we can.

Sorry. While I do think the decision to use AST may still be something we'll regret, it's not actually something I think we should revisit now, and I apologize for bringing it up.

The important argument here is that the fact that old Wcs class mixed implementation and interface turned out to be a major design flaw, and (as I understand it) a major reason @r-owen has had to do so much work on DM-10764 to update the stack to use a different implementation that shouldn't have needed to change [much of] the interface. (Some of the work also came from us making additional desirable interface changes at the same time, which perhaps should have been worked in on other timescales - hindsight is 20/20). I just don't want to see us make the same mistake again just because we're more confident now that the new implementation will do all we ever need.

r-owen · 2017-12-05T20:08:13Z

A few comments for @TallJimbo :

I have eliminated makeWcs. All code calls the SkyWcs constructor directly to construct a WCS from metadata or a standard (no distortion) FITS WCS. A free function is used to construct a TAN-SIP WCS, since I wanted to avoid clutter and there are two flavors: one with inverse coefficients and one without.
There are a number of functions that take a SkyWcs and modify it in some way, returning a new SkyWcs, for instance returning a pixelsToIwc transform or inserting a Transform. Those are the functions I worry about as to making a new kind of WCS -- they rely on SkyWcs to work and I worry about how we'd implement them for some other kind of WCS.
I have added one transform to AST and can add others. It is more efficient to ask David Berry to do it, and I hope he will be available to do so until commissioning. I agree it should be easier, but let's not overstate how hard it is.

It referred to PIXEL and TAN_PIXEL, but the coordinate systems have an "S" on the end: PIXELS and TAN_PIXELS.

Remove unnecessary use of enpoints from one test case.

that explains why coord is not imported -- list the right ticket to resolve before deleting the commented-out import entirely

This is the preferred way to wrap table persistence support within the afw package, as it eliminates a significant source of circular import issues. It also simplifies the pybind11 wrappers a bit, and so may be preferred everywhere.

Make member functions that returned a Transform (readStream, readString and then) return a shared_ptr<Transform>

Don't import more than is wanted. Fixed in two unit tests.

Document the need for subclasses of Persistable to override getPythonModule.

Add excludeNames argument to formatFitsProperties(PropertySet)

Polygone has one constructor and one method (transform) that take an XYTransform; add an analogous constructor and overload of transform that take a TransformPoint2ToPoint2.

These symbols are are now available in lsst.afw.geom

Add lsst/afw/geom/detail/wcsUtils.h and more public Python utilities to lsst.afw.geom.wcsUtils.py Some of these are copied from afw::image. The rest are new and are intended to support afw::geom::SkyWcs.

Add makeFitsHeaderFromMetadata, makeSipIwcToPixel and makeSipPixelToIwc functions to testUtils.py. These are used (and thus indirectly tested) in test_skyWcs.py

- Rename makeTanWcsMetadata to makeSimpleWcsMetadata and add the `projection` argument, which defaults to "TAN". - Change `readFitsWcs` to accept a `PropertySet` instead of requiring a `PropertyList`. - Add `getPropertyListFromFitsChan` - Remove `getSkyFrame` - Improve error handling - Improve the documentation

and remove the virtual empty destructor and virtual modifiers on member functions.

Pass the frameSet pointer by value instead of &&

instead of Transform

It was importing headers the were not used, and not importing headers that were used (though pulled in by transformFactory.h)

Instead of inheriting from Transform, make it contain a FrameDict. Add FITS binary table persistence. Add missing methods and free functions that replace old Wcs methods. Add new makeSkyWcs functions. Remove SkyWcs(crpix, crval, ...) constructor in favor of makeSkyWcs. Change member functions and free functions that returned a SkyWcs to return a shared_ptr<SkyWcs>.

r-owen added 2 commits November 21, 2017 10:47

Fix bug in assertCoordListsAlmostEqual

edb784a

The error string was mis-formatted

Fix typo in Transform.h

babad4d

TallJimbo requested changes Nov 30, 2017

View reviewed changes

r-owen added 20 commits December 8, 2017 09:50

Remove obsolete comment from SpherePoint.h

e33ec44

Fix documentation of makePixelToTanPixel

0e1022d

It referred to PIXEL and TAN_PIXEL, but the coordinate systems have an "S" on the end: PIXELS and TAN_PIXELS.

Add note about units to Transform.getJacobian doc

01ecc3f

Simplify a test in test_transform.py

6e257c3

Remove unnecessary use of enpoints from one test case.

Fix warning in test_wcsFitsTable.py

a0590cd

Use TransformPoint2ToPoint2 typedef

3b0372a

Add simplify=true argument to Transform::then

26c8fea

Remove unused var in ostream operator<<(Transform)

6c8502b

Standardize comment style in Endpoint.h and Transform.h

f71d602

Fix comment in transform.cc

c6963fe

Endpoint: refine comment

8e42d93

that explains why coord is not imported -- list the right ticket to resolve before deleting the commented-out import entirely

Add addPersistableMethods to table/io/python.h

d02785a

This is the preferred way to wrap table persistence support within the afw package, as it eliminates a significant source of circular import issues. It also simplifies the pybind11 wrappers a bit, and so may be preferred everywhere.

Modernize wrapping of table persistence

3bab58e

Remove table.io import from photoCalib.cc

cee5b52

Hoist Ellipse, Quadrupole and polygon symbols into lsst.afw.geom

9dba51f

Transform: member functions return shared_ptr

a4f6bea

Make member functions that returned a Transform (readStream, readString and then) return a shared_ptr<Transform>

Import astropy.io.fits instead of astropy.io

c06f8a6

Don't import more than is wanted. Fixed in two unit tests.

Document override getPythonModule in Persistable

70b05b5

Document the need for subclasses of Persistable to override getPythonModule.

Enhance formatFitsProperties

6932b6a

Add excludeNames argument to formatFitsProperties(PropertySet)

Fix documentation of warpImage

29dc11a

r-owen added 10 commits December 12, 2017 15:59

Add TransformPoint2ToPoint2 support to Polygon

f93a35d

Polygone has one constructor and one method (transform) that take an XYTransform; add an analogous constructor and overload of transform that take a TransformPoint2ToPoint2.

Simplify import of Ellipse, Polygon, Quadrupole

1b3ee89

These symbols are are now available in lsst.afw.geom

Add geom wcsUtils

e2393f9

Add lsst/afw/geom/detail/wcsUtils.h and more public Python utilities to lsst.afw.geom.wcsUtils.py Some of these are copied from afw::image. The rest are new and are intended to support afw::geom::SkyWcs.

Expand geom/testUtils.py

c255d26

Add makeFitsHeaderFromMetadata, makeSipIwcToPixel and makeSipPixelToIwc functions to testUtils.py. These are used (and thus indirectly tested) in test_skyWcs.py

Make Transform final

6aac285

and remove the virtual empty destructor and virtual modifiers on member functions.

Transform: simplify shared_ptr constructor

8d67bab

Pass the frameSet pointer by value instead of &&

Make functions return shared_ptr<Transform>

1d2ea93

instead of Transform

Clean up imports in transformFactory.cc

55d4a10

It was importing headers the were not used, and not importing headers that were used (though pulled in by transformFactory.h)

r-owen force-pushed the tickets/DM-12764 branch from 66afc75 to d786b0e Compare December 13, 2017 01:25

r-owen merged commit d786b0e into master Dec 13, 2017

ktlim deleted the tickets/DM-12764 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12764: Overhaul SkyWcs #294

DM-12764: Overhaul SkyWcs #294

r-owen commented Nov 30, 2017

TallJimbo left a comment •

edited

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017 •

edited

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017

TallJimbo Dec 5, 2017

r-owen Dec 7, 2017

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017 •

edited

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017

TallJimbo Nov 30, 2017

r-owen Dec 4, 2017

TallJimbo Nov 30, 2017

TallJimbo Nov 30, 2017

TallJimbo Nov 30, 2017

TallJimbo commented Nov 30, 2017

r-owen commented Dec 4, 2017

TallJimbo commented Dec 5, 2017 •

edited

timj commented Dec 5, 2017

TallJimbo commented Dec 5, 2017

TallJimbo commented Dec 5, 2017

r-owen commented Dec 5, 2017 •

edited

DM-12764: Overhaul SkyWcs #294

DM-12764: Overhaul SkyWcs #294

Conversation

r-owen commented Nov 30, 2017

TallJimbo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Dec 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Dec 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Nov 30, 2017

r-owen commented Dec 4, 2017

TallJimbo commented Dec 5, 2017 • edited

timj commented Dec 5, 2017

TallJimbo commented Dec 5, 2017

TallJimbo commented Dec 5, 2017

r-owen commented Dec 5, 2017 • edited

TallJimbo left a comment •

edited

r-owen Dec 4, 2017 •

edited

r-owen Dec 4, 2017 •

edited

TallJimbo commented Dec 5, 2017 •

edited

r-owen commented Dec 5, 2017 •

edited