Fix Version/s: None
Sprint:Alert Production S17 - 3, Alert Production S17 - 4, Alert Production S17 - 5
I originally designed astshim to use a transposed version of data lists because we expected to be able to provide that with live views of PointList and SpherePointList (and we had some hope AST could be updated to support transposed data directly). Both of these have proven false, so it makes much more sense to use AST's standard data order in astshim. This will be more efficient (it avoids a copy on input and output) and will be less surprising to users.
Overall looks good to me, a few minor comments up on github. Nothing show stopping.
I forgot to make a pull request for afw. I've now done that. Please have a look.
I got rid of Point3Endpoint because we have no identified need for it and adding it requires additional code (which was not the case when raw data was transposed). However, for the record, here are some ideas for adding back later (or if Nate Lust or others strongly prefer, on this ticket branch):
- Add a constructor to Point<T, N> that takes a column view of an ndarray. This is easy to use correctly and efficient, but does add another constructor to the API of Point<T, N>.
- Add a "strides" argument to the raw data pointer constructor that defaults to 1. This was Jim Bosch's suggestion. it is very simple to implement and adds a minimum of API clutter, but I worry that it is too easy to get the value of strides wrong.
- Add two new classes: Point3Endpoint and template<N> BasePointEndpoint, to hold code common to Point2Endpoint and Point3Endpoint. This needs no changes to the Point classes but I feel it is a bit too messy.
- Copy the data for each point from the 2D raw array into a C array, then use the C array to construct the Point. Krzysztof Findeisen suggested this. It is very simple but requires an extra copy for each axis of each point. I worry about the performance hit, though it would surely be small and perhaps negligible.
Nothing caught my eye as worth addressing in the afw branch, so the only comments I have are for the other one. One minor note is that officially all comments are supposed to be complete sentences with punctuation etc, but I don't think it is worth taking a second look at.
Changes include in astshim:
and in afw: