Having thought some more, I think we want to have the match behaviour configurable on the basis of a function parameter because I think there are times when you may want that exception and there are times when you really don't.
Also, MatchOptimisticBTask.loadAndMatch is filtering the sources, and doing a solve-and-match as opposed to a plain match. For cases where we want a plain match with the existing astrometric solution for QA (as in measureCoaddSources.py and maybe in other cases like in characterizeImage and imageDifference), we want just a plain matching task that will load the reference sources and use lsst.afw.table.matchRaDec. I'll add something like that into pipe_tasks.

ANetBasicAstrometryTask.useKnownWcs is basically the implementation of the higher-level AstrometryTask.loadAndMatch. It does not attempt to solve for an improved WCS and I agree that both should return an empty list if there are no matches found instead of raising an exception (and I don't think that needs to be controllable by a flag):

In anetAastrometry.py I think you can just remove or len(matches == 0) from the following:

            if matches is None or len(matches) == 0:

                raise RuntimeError("No astrometric matches")

I don't see anything similar in AstrometryTask but it may be buried. Some testing is in order, at least.

Both of these tasks also have a solve method that attempts to fit an improved WCS. I believe, and I hope you agree, that fitting should fail with an exception if too few matches, are found. So...If you update ANetAstrometryTask as above, then should a test be added to its solve method, or does it already handle that condition cleanly?

Regarding loadAndMatch... some filtering is essential. For instance if we match to a deep catalog of sources we found that a S/N cut was essential for getting a decent match. However, I think you are probably right that it should not further cut the list down (only leaving "good" sources). I think that is legacy code; as I recall the HSC code I started from matched both "good" sources and "usable" sources, and picked whichever solution was better, and if it was the "usable" sources it then purged the list (as we do now) to leave only the "good" sources. With the other changes made, we found that the double matching wasn't necessary, so now "good" source list is never used for matching, but the purge of non-good sources remains. I agree that should be removed. It will take some work to make sure that removing that final cut doesn't break code that uses the matcher. I suspect that would be better handled on a separate ticket, in order to keep related functionality together, but you could probably squeeze it into this ticket if you really want to.

Regarding the loader solving for a WCS: the only solving that I know if is the basic shifting and rotating needed to match (and the solution does not affect the input WCS; it's an internal detail). I don't think there is anything we can or should change about that, but perhaps I am missing something.

If I want a list of all matches using the current Wcs, then I don't want any of "the basic shifting and rotating needed to match". Furthermore, in that case I want all matches, not just those that have survived the filtering. It's become clear to me that this function is different from that envisioned for the AstrometryTask.

Russell Owen, would you review this please? There are changes in meas_astrom, pipe_tasks and obs_subaru. I looked in other obs packages for configuration overrides for measureCoaddSources, and didn't see any. This has gone through Jenkins (including ci_hsc) successfully.

price@price-laptop:~/LSST/meas/astrom (tickets/DM-7117=) $ git sub

commit 763414b3cf53d3bed29d73b04ad014f1fbd80e04

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:04:40 2016 -0400

    createMatchMetadata: add additional interface

    The 'match metadata' stores data we need to regenerate the matches

    (i.e., cone search parameters). The current interface uses a bbox

    and Wcs, and converts those to the cone search parameters that are

    stored in the metadata; but there has not been an interface to

    construct the 'match metadata' given the cone search parameters.

    Created a new class, MatchMetadata for the purpose of simplifying

    the creation of the 'match metadata'. Chose to use a class rather

    than a new factory function because it's more fundamental. The old

    interface now uses the new class.

 python/lsst/meas/astrom/createMatchMetadata.py | 31 +++++++++++++++++---------

 1 file changed, 20 insertions(+), 11 deletions(-)

commit bc210ed0d8bc2966e28d15dbcf5641f1df83bb46

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:21:14 2016 -0400

    add task to perform simple matching to a reference catalog

    The two AstrometryTasks (MatchOptimisticBTask and ANetAstrometryTask)

    support matching, but these are optimised for determining astrometric

    solutions so there are a variety of contortions involved, including

    allowing rotations and distortions and filtering of the input catalog.

    This new MatchTask is much simpler:

    * no exposure required, only a catalog;

    * no rotations or translations are permitted, only a straight match;

    * no filtering of the input catalog, so all matches are returned; and

    * no exception is raised when there are no matches.

    This makes it suitable for gathering a list of matches for QA purposes.

 python/lsst/meas/astrom/__init__.py |   1 +

 python/lsst/meas/astrom/match.py    | 102 ++++++++++++++++++++++++++++++++++++

 2 files changed, 103 insertions(+)

price@price-laptop:~/LSST/pipe/tasks (tickets/DM-7117=) $ git sub

commit bb05a2999a73a1821e857597aa8555642b8f53e3

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:29:47 2016 -0400

    multiband: make pyflakes-clean

    Imported symbol wasn't used.

 python/lsst/pipe/tasks/multiBand.py | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

commit 0876e4c753478fc19ea4135e35adbea4b5737f44

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:36:31 2016 -0400

    multiband: use new MatchTask

    For measureCoaddSources, it's helpful (for QA especially) to match

    our catalog against a reference catalog. We were using an AstrometryTask

    for this, but this isn't the right choice because they can allow

    rotation and translation, filtering of our catalog, and they raise an

    exception if the matching fails. And if you accidentally allow it to

    fit a Wcs (which we have done in the past!), it can cause trouble for

    your catalog. We just want a simple matching against the reference

    catalog, which is provided by the new MatchTask.

 python/lsst/pipe/tasks/multiBand.py | 14 ++++++--------

 1 file changed, 6 insertions(+), 8 deletions(-)

price@price-laptop:~/LSST/obs/subaru (tickets/DM-7117=) $ git sub

commit bd26ea81bf2cdc84e431e1167296d5a6eab3e9a8

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:47:44 2016 -0400

    config: adapt to usage of new MatchTask in measureCoaddSources

    The refObjLoader is an extra level down now.

    This cleaned up the matchOptimisticB vs ANetAstrometry problem!

 config/hsc/measureCoaddSources.py | 10 ++--------

 config/measureCoaddSources.py     |  9 ++-------

 2 files changed, 4 insertions(+), 15 deletions(-)

commit b8b0aead2bcdbb677787d22fb3392b9995735dff

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 21:34:21 2016 -0400

    config: fix syntax mistake in exception catching

    Due to old python syntax, the line

        except KeyError, ImportError:

    is interpreted the same as

        except KeyError as ImportError:

    which is not what was intended, and causes the process

    to die if meas_modelfit isn't setup.

 config/cmodel.py | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

I am happy to review this, but won't be able to get to it until Tuesday at the earliest.

I have two substantive concerns:

1) I would expect the new matcher to have the same API as the existing matcher (MatchOptimisticBTask), and that to use it one would retarget the matcher in AstrometryTask and call loadAndMatch. I think this would be clearer to users, since we then simply have more than one matcher. Can you please explain why you chose such a different route?

2) Please add a unit test for the new task.

Aside from those concerns, I'm happy. I added a few minor suggestions to github, but overall the new code is clear and well documented, and I it looks generally useful beyond the use case it was designed for.

1) I didn't feel constrained to use the same API as for the AstrometryTask subclasses because this isn't an AstrometryTask. It doesn't do any fitting by design, and doesn't care about astrometry (no wcs anywhere!), so it shouldn't be an AstrometryTask. This is a different beast. I would be happy to name it something different, but I think the difference between MatchTask and AstrometryTask is fairly clear. If anything, confusion would come because of the name of MatchOptimisticBTask doesn't make it clear it's an AstrometryTask (OptimisticBAstrometryTask?).

2) I'll add a unit test and address the issues on GitHub.

Russell Owen, I've added a unit test, made the small changes you asked about, and made a couple of small fixes to the MatchTask. Would you like to look at this again? The test adds 95 lines of new code.

pprice@tiger-sumire:~/LSST/meas/astrom (tickets/DM-7117=) $ git sub

commit 763414b3cf53d3bed29d73b04ad014f1fbd80e04

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:04:40 2016 -0400

    createMatchMetadata: add additional interface

    The 'match metadata' stores data we need to regenerate the matches

    (i.e., cone search parameters). The current interface uses a bbox

    and Wcs, and converts those to the cone search parameters that are

    stored in the metadata; but there has not been an interface to

    construct the 'match metadata' given the cone search parameters.

    Created a new class, MatchMetadata for the purpose of simplifying

    the creation of the 'match metadata'. Chose to use a class rather

    than a new factory function because it's more fundamental. The old

    interface now uses the new class.

 python/lsst/meas/astrom/createMatchMetadata.py | 31 +++++++++++++++++---------

 1 file changed, 20 insertions(+), 11 deletions(-)

commit f66b07f1cb8ad71a2cbdb715ced179b23e8b5fb7

Author: Paul Price <price@astro.princeton.edu>

Date:   Wed Aug 3 20:21:14 2016 -0400

    add task to perform simple matching to a reference catalog

    The two AstrometryTasks (MatchOptimisticBTask and ANetAstrometryTask)

    support matching, but these are optimised for determining astrometric

    solutions so there are a variety of contortions involved, including

    allowing rotations and distortions and filtering of the input catalog.

    This new MatchTask is much simpler:

    * no exposure required, only a catalog;

    * no rotations or translations are permitted, only a straight match;

    * no filtering of the input catalog, so all matches are returned; and

    * no exception is raised when there are no matches.

    This makes it suitable for gathering a list of matches for QA purposes.

 python/lsst/meas/astrom/__init__.py |   1 +

 python/lsst/meas/astrom/match.py    | 105 ++++++++++++++++++++++++++++++++++++

 2 files changed, 106 insertions(+)

commit 256d30525e3237ccec92a9f56d3c1c2224fed237

Author: Paul Price <price@astro.princeton.edu>

Date:   Tue Aug 9 21:02:26 2016 -0400

    Add test for MatchTask

 tests/testMatch.py | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++

 1 file changed, 95 insertions(+)

The new unit test looks great (I put two minor suggestions on github).

Regarding the API discussion: I see your point about astrometry vs. matching, but here is a point to consider. Suppose we want to run ProcessCcd on data for which we already have an excellent WCS (e.g. SDSS data). A logical thing to do would be to plug your simple matcher into the astrometry task and disable WCS fitting by setting forceKnownWcs=True. At that point we'd get simple matching to the reference catalog for the photometric fit. As it stands we have to use a fitter that does more than is ideal.

Perhaps you could have it both ways by supporting the matchObjectsToSources method of MatchOptimisicBTask and returning the same struct. Then your new task could be used both ways.

You also reminded me of something I missed the first time: "there is no WCS anywhere". Your matcher requires that the "coord" field of the source catalog be set. This is not the case for all persisted source catalogs (e.g. icSrc), so please document this requirement. You might also consider making it optional by adding support for the task to set the coord field using a supplied wcs. If you do choose to support matchObjectsToSources then that will fall out naturally, since wcs is one of the arguments.

At risk of bike shedding, please also consider is a slight rename to SimpleMatchTask, or something similar, in order to better distinguish it from other existing matching tasks, e.g. MatchOptimisticBTask. If you feel it would be better to rename MatchOptimisticBTask instead, you could do that instead (though I worry that adding Astrometry into the name will make that confusing with respect to AstrometryTask). In any case, I worry that the current situation of having MatchTask and MatchOptimisticBTask is too confusing. Names are a pain.

One more note: on github (on a commit that no longer exists) you said:

I think you're referring to the line in astrometry.py:

    if filterName == "_unknown_":

        filterName = None

I have no idea where the "_unknown_" comes from, and I've never seen it in practise. I think you added that code; do you know what it's for?

"_unknown_" is ancient; I just copied it from existing code when I moved the metadata setting code into its own function. Based on a global search of the code base, I suspect it comes from this in Filter.cc:

namespace {

    std::string const unknownFilter = "_unknown_";

}

it also showed up in some saved test results from obs_cfht, so I think the name is sometimes used.

Perhaps the best name to use for an unknown filter in your new code is afw.image.unknownFilter instead of the explicit "UNKNOWN" that you are using? In any case, the code on master omits the FILTER property if the filter name is unknown, but your new code now sets it to something, so I hope you are sure that this is a desirable and safe change.

I'm glad you've pushed me to think about these things because I went and discovered that the AstrometryTask is laid out a bit differently than I remembered, and divided into loader, matcher and fitter components; so now I think I understand your nudges better.

I appreciate the desire to make MatchTask more broadly useful but I would like to leave it as it is for now because:
1. I think this work is already over its time budget.
2. The work is already useful as it stands, and improvements can be made later.
3. The API is quite different from what would be required (e.g., refObjLoader and matchRadius are config parameters instead of being provided through the function API, so adapting to the required API would effectively strip out all the other code that makes it useful for its current job of loading and matching). I think this comes from the different stated aims (match for QA vs match for astrometry fitting).
4. I expect changes will be made within the AstrometryTask and its children to fix criticisms that have accumulated, and so work done to make MatchTask usable within AstrometryTask would need to be redone.

Now that I understand that there is a matcher within the AstrometryTask, I agree that a better name and better documentation would be helpful in distinguishing it as incompatible.

I propose renaming the MatchTask to DirectMatchTask and adding docs indicating that it is currently incompatible with the AstrometryTask. I'm working on this now.

Changes are in. I'm working on getting this to pass ci_hsc, and then will merge. Last chance to object!

Jenkins passed. I'm about to merge.

Brain was on autopilot while merging: I neglected to squash the commits. Too eager to get it done, sorry.

Merged to master stupidly. ):

Paul Price, could you please add a brief comment on this to the release notes (https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+F16+Release+Notes)? Thanks.

Done.