Changes in meas_algorithms on tickets/DM-2511:

• Added free function setMatchDistance to set the distance field of a collection of matches.
• Added a unit test for the new function.

Changes in meas_algorithms on tickets/DM-2511:

• Updated MatchOptimisticBTask and FitTanSipWcsTask to set the distance field of the match list.
• Updated FitTanSipWcsTask to set the coord of each source in matches even if a source catalog not supplied. Log a warning if this happens.
• Updated FitTanSipWcsTask to set the centroid of each reference object in matches even if a reference catalog not supplied. Log a warning if this happens.
• Update match and fit unit tests to confirm that distance is correctly set.

Beside the comments that I put in the pull request, I have a general comment here which is more a design question :
Is it a good idea to implement a new function in meas_algorithms for such a simple thing ? My worry is that by multiplying this kind of very simple functions we may end up with a very fragmented code where it is very difficult to navigate. The consequence is also that the unit test looks somewhat disproportionate with what you are checking.

In response to Dominique Boutigny's question about implementing a new function in meas_algorithms: you raise an excellent point. I have several reasons for preferring a single centralized implementation:

• It is needed in more than one place now, and future WCS fitters may also need it.
• The only tricky part about the algorithm is obtaining the necessary data from the catalogs. The method for doing that has changed at least once, so I would prefer to centralize the code.
• It may want to be implemented in C++ at some point.

I chose to put it in meas_algorithms instead of meas_astrom because I imagined a few algorithms might want it that would not want to depend on meas_astrom. Jim Bosch suggested it might go into afw::table since it has no external dependencies.

Seems to me this should be a method on afw.table.Match (no particular reason it has to be limited to ReferenceMatch), perhaps taking "coord" as a parameter. It can be implemented in Python in Match.i, rather than C++. Is this a viable option?

Adding a method to afw.table.Match to set the distance is an interesting idea. I'm not entirely convinced it's a good idea, but its weaknesses may expose the weaknesses of setMatchDistance itself.

The main problem is that there is more than one way to compute that distance, especially since sometimes a match consists of a reference object and a source, and sometimes it consists of a pair of sources. Here are some possibilities:

• If the coord of match.first and match.second is known, that is arguably simplest (no need to know plate scale) and most accurate (it works near the poles). That is what I coded for and is the case in meas_astrom but may well not be the case for all Matches.
• If the centroids of first and second objects are known then one can use those and an approximation for the plate scale, and in that case one may want to allow flexibility as to which centroid fields are used, especially since sources and reference objects have different field names for centroids.
• If first is a reference object and second is a source, one could imagine supplying a wcs and using that to tie the coord of the reference object and the centroid of the source together.
• Pre-DM-2511 code may or may not set the distance field, but if it sets the distance field it sets it directly, presumably using one of the techniques above.

So maybe this function should go in meas_astrom where I can assume that coord fields exist and are properly set for match.first and match.second.

I would be quite open to reconsidering how the distance field in the Match objects works, as I think it's a bit of a mess that the distance field can currently mean either pixels or angular distance. Maybe we could turn it into an Angle, and have it always represent angular distance. Give matchXy an optional pixel scale parameter, and leave the match distance NaN if it's not provided.

Also, if the input catalogs are SourceCatalogs or SimpleCatalogs, they're guaranteed to have coord fields (or, in the near future with DM-1766, "coord_ra" and "coord_dec" fields that can be accessed via a CoordKey). So there's no need to go to meas_astrom for that reason.

I think the distance is always meant to be radians on the sky. Thus if distance is set from centroids, it must take the plate scale into account. I think it should be an Angle and that it should be better documented.

The current design feels messy and dangerous to me. It is far too easy for the distance field to be unset or set incorrectly. This is especially a problem while fitting a WCS: the distance might be valid for an older version of the WCS. This whole issue of consistency is deeper than Match, though. It originates in sources and reference objects. I think something like the following would be a huge improvement:

• Source and reference object catalogs should have an pointer to a WCS.
• Each catalog entry has access to this WCS.
• Source coord is computed from slot centroid and catalog WCS.
• Reference centroid is computed from the reference coord and catalog WCS.
• Once this is done, then match distance can similarly be reliably computed at need.

Subtleties:

• One might cache the computed values, but I think this would be too difficult to be worth the effort.
• Handling a null WCS pointer requires some care. I think for catalogs that the computed field values should be NaNs, since that matches other field access. Perhaps an associated flag would help. Catalogs can have a method "hasWcs", but sometimes users only have individual entries (e.g. in the case of matches), not the full catalog.

Meanwhile, is there a reasonable short-term solution for DM-2511 that will not leave us with legacy code we regret later? If a design such as I just mentioned sounds reasonable then we won't need the function in question in this ticket and I could move it to meas_astrom where it will be relatively out of the way. I could even hide it in meas.astrom.detail (I am loath to make it private, because I want a unit test).

Based on HipChat discussion I have moved setMatchDistance to meas_astrom. I implemented all other requested changes.