[DM-3633] Add readMatches back to meas_astrom - Jira

Russell Owen created issue - 28/Aug/15 2:53 PM

Russell Owen made changes - 28/Aug/15 2:54 PM

Field	Original Value	New Value
Description	Recent changes to meas_astrom accidentally removed a function readMatches (copied below). Please restore it, preferably in its own module (though if someday we have more small python functions we may want a utils.py module). {code} def readMatches(butler, dataId, sourcesName='icSrc', matchesName='icMatch', config=MeasAstromConfig(), sourcesFlags=afwTable.SOURCE_IO_NO_FOOTPRINTS): """Read matches, sources and catalogue; combine. @param butler Data butler @param dataId Data identifier for butler @param sourcesName Name for sources from butler @param matchesName Name for matches from butler @param sourcesFlags Flags to pass for source retrieval @returns Matches """ sources = butler.get(sourcesName, dataId, flags=sourcesFlags) packedMatches = butler.get(matchesName, dataId) astrom = Astrometry(config) return astrom.joinMatchListWithCatalog(packedMatches, sources) {code}	Recent changes to meas_astrom accidentally removed a function readMatches (copied below). Please restore it, preferably in its own module (though if someday we have more small python functions we may want a utils.py module). {code} def readMatches(butler, dataId, sourcesName='icSrc', matchesName='icMatch', config=MeasAstromConfig(), sourcesFlags=afwTable.SOURCE_IO_NO_FOOTPRINTS): """Read matches, sources and catalogue; combine. @param butler Data butler @param dataId Data identifier for butler @param sourcesName Name for sources from butler @param matchesName Name for matches from butler @param sourcesFlags Flags to pass for source retrieval @returns Matches """ sources = butler.get(sourcesName, dataId, flags=sourcesFlags) packedMatches = butler.get(matchesName, dataId) astrom = Astrometry(config) return astrom.joinMatchListWithCatalog(packedMatches, sources) {code} Also include a unit test.

Russell Owen made changes - 28/Aug/15 2:54 PM

Story Points

1

2

Russell Owen made changes - 28/Aug/15 2:55 PM

Description

Recent changes to meas_astrom accidentally removed a function readMatches (copied below). Please restore it, preferably in its own module (though if someday we have more small python functions we may want a utils.py module).
{code}
def readMatches(butler, dataId, sourcesName='icSrc', matchesName='icMatch', config=MeasAstromConfig(), sourcesFlags=afwTable.SOURCE_IO_NO_FOOTPRINTS):
    """Read matches, sources and catalogue; combine.
    @param butler Data butler
    @param dataId Data identifier for butler
    @param sourcesName Name for sources from butler
    @param matchesName Name for matches from butler
    @param sourcesFlags Flags to pass for source retrieval
    @returns Matches
    """
    sources = butler.get(sourcesName, dataId, flags=sourcesFlags)
    packedMatches = butler.get(matchesName, dataId)
    astrom = Astrometry(config)
    return astrom.joinMatchListWithCatalog(packedMatches, sources)
{code}

Also include a unit test.

Recent changes to meas_astrom accidentally removed a function readMatches (copied below). Please restore it, preferably in its own module (though if someday we have more small python functions we may want a utils.py module).

Also please include a unit test.

{code}
def readMatches(butler, dataId, sourcesName='icSrc', matchesName='icMatch', config=MeasAstromConfig(), sourcesFlags=afwTable.SOURCE_IO_NO_FOOTPRINTS):
    """Read matches, sources and catalogue; combine.
    @param butler Data butler
    @param dataId Data identifier for butler
    @param sourcesName Name for sources from butler
    @param matchesName Name for matches from butler
    @param sourcesFlags Flags to pass for source retrieval
    @returns Matches
    """
    sources = butler.get(sourcesName, dataId, flags=sourcesFlags)
    packedMatches = butler.get(matchesName, dataId)
    astrom = Astrometry(config)
    return astrom.joinMatchListWithCatalog(packedMatches, sources)
{code}

Lauren MacArthur made changes - 02/Dec/15 1:20 PM

Assignee

Lauren MacArthur [ lauren ]

Lauren MacArthur made changes - 02/Dec/15 4:27 PM

Status

To Do [ 10001 ]

In Progress [ 3 ]

John Swinbank made changes - 04/Dec/15 6:23 AM

Epic Link

~~DM-3585~~ [ 19682 ]

John Swinbank made changes - 04/Dec/15 6:23 AM

Sprint		Science Pipelines DM-W16-4 [ 187 ]
Story Points	2	4
Team		Data Release Production [ 10301 ]

Hide

Permalink

Lauren MacArthur added a comment - 16/Dec/15 3:44 PM

Russell Owen & Paul Price: In reinstating this function, I realized it is a bit odd as its only real job is to delegate to joinMatchListWithCatalog(). That function lives in anetBasicAstrometry.py, so even if the caller has initialized AstrometryTask, calling this function still requires initializing an ANetBasicAstrometryTask. I've come up with two solutions and have pushed branches for both:

u/lauren/~~DM-3633~~alt:
Just adds back the function in its own file and slightly simplifies the API (i.e. it has the caller pass in the matches and source lists themselves rather than reloading them here through a passed in butler).

u/lauren/~~DM-3633~~
Moves joinMatchListWithCatalog() to setMatchDistance.py. The user can either pass in their own astrometry task instance, or one will be created for them. This has the advantage that one no longer needs to go through ANetBasicAstrometryTask to get to joinMatchListWithCatalog(). It also comes with the added bonus that the (adapted) unittest SourceMatchJoin.py already provides a unit test. Note that I also made the following filename changes:
setMatchDistance.py -> matchUtils.py
SourceMatchJoin.py -> testJoinMatchListWithCatalog.py

I prefer the latter, but if you have strong opinions either way, let me know. I'll wait until we've settled on one or the other solution before officially putting this up for review (shout if you can/are willing to take it!).
Oh, and a Jenkins build was successful on the u/lauren/DM-3633 branch.

Show

Lauren MacArthur added a comment - 16/Dec/15 3:44 PM Russell Owen & Paul Price : In reinstating this function, I realized it is a bit odd as its only real job is to delegate to joinMatchListWithCatalog() . That function lives in anetBasicAstrometry.py , so even if the caller has initialized AstrometryTask , calling this function still requires initializing an ANetBasicAstrometryTask . I've come up with two solutions and have pushed branches for both: u/lauren/ DM-3633 alt: Just adds back the function in its own file and slightly simplifies the API (i.e. it has the caller pass in the matches and source lists themselves rather than reloading them here through a passed in butler). u/lauren/ DM-3633 Moves joinMatchListWithCatalog() to setMatchDistance.py . The user can either pass in their own astrometry task instance, or one will be created for them. This has the advantage that one no longer needs to go through ANetBasicAstrometryTask to get to joinMatchListWithCatalog() . It also comes with the added bonus that the (adapted) unittest SourceMatchJoin.py already provides a unit test. Note that I also made the following filename changes: setMatchDistance.py -> matchUtils.py SourceMatchJoin.py -> testJoinMatchListWithCatalog.py I prefer the latter, but if you have strong opinions either way, let me know. I'll wait until we've settled on one or the other solution before officially putting this up for review (shout if you can/are willing to take it!). Oh, and a Jenkins build was successful on the u/lauren/ DM-3633 branch.

Hide

Permalink

Russell Owen added a comment - 16/Dec/15 4:28 PM - edited

I have a slightly different suggestion: make joinMatchListWithCatalog (or readMatches, but I feel the other name is clearer) it a method on LoadReferenceObjectsTask. Do not have a free function at all. Advantages:

It will easily work with any kind of reference catalog, since LoadReferenceObjectsTask is the base class for all reference object loaders.
The method only requires a reference loader, so it is a natural fit.

If you prefer your solutions, then I agree that u/lauren/DM-3633 is better, but I would make the following changes:

Pass in an instance of a reference object loader task.This is significantly more general than passing in an astrometry task, and requires making fewer assumptions about the object. It is also the reason that I think it makes more sense for this function to be a method on LoadReferenceObjectsTask.
Do not accept None as a value for this function. I feel that is too dangerous, especially as it assumes a particular catalog format (a.net index files).

Show

Russell Owen added a comment - 16/Dec/15 4:28 PM - edited I have a slightly different suggestion: make joinMatchListWithCatalog (or readMatches , but I feel the other name is clearer) it a method on LoadReferenceObjectsTask . Do not have a free function at all. Advantages: It will easily work with any kind of reference catalog, since LoadReferenceObjectsTask is the base class for all reference object loaders. The method only requires a reference loader, so it is a natural fit. If you prefer your solutions, then I agree that u/lauren/ DM-3633 is better, but I would make the following changes: Pass in an instance of a reference object loader task.This is significantly more general than passing in an astrometry task, and requires making fewer assumptions about the object. It is also the reason that I think it makes more sense for this function to be a method on LoadReferenceObjectsTask . Do not accept None as a value for this function. I feel that is too dangerous, especially as it assumes a particular catalog format (a.net index files).

Hide

Permalink

Lauren MacArthur added a comment - 18/Dec/15 12:22 PM

Russell, would you mind giving this a review? I have implemented your first suggestion above and the changesets are in u/lauren/DM-3633 branches on meas_astrom and meas_algorithms. I have tested that results of this implementation are exactly the same as the other two on the output of a processCcd.py run on HSC data. The joinMatchListWithCatalog() function is tested in the testJoinMatchListWithCatalog.py unittest in meas_astrom, so it will be tested in the context of CI. It is unfortunate that it is not easy to test it in meas_algorithms itself, but there is a precedent for such a case: the loadSkyCircle() function in meas_algorithms (on which joinMatchListWithCatalog() relies) is also only tested in meas_astrom's testLoadAstrometryNetObjects.py.

A Jenkins build succeeded.

Show

Lauren MacArthur added a comment - 18/Dec/15 12:22 PM Russell, would you mind giving this a review? I have implemented your first suggestion above and the changesets are in u/lauren/ DM-3633 branches on meas_astrom and meas_algorithms . I have tested that results of this implementation are exactly the same as the other two on the output of a processCcd.py run on HSC data. The joinMatchListWithCatalog() function is tested in the testJoinMatchListWithCatalog.py unittest in meas_astrom , so it will be tested in the context of CI. It is unfortunate that it is not easy to test it in meas_algorithms itself, but there is a precedent for such a case: the loadSkyCircle() function in meas_algorithms (on which joinMatchListWithCatalog() relies) is also only tested in meas_astrom 's testLoadAstrometryNetObjects.py . A Jenkins build succeeded.

Lauren MacArthur made changes - 18/Dec/15 12:22 PM

Reviewers		Russell Owen [ rowen ]
Status	In Progress [ 3 ]	In Review [ 10004 ]

Hide

Permalink

Russell Owen added a comment - 18/Dec/15 3:33 PM - edited

This looks great.

My one request is to improve the documentation for the moved method. It was already confusing (and not PEP-8 compliant and did not work with Doxygen). But it makes even less sense now, because some of the things it referred to were part of its old class. The following might be a reasonable start, but feel free to ignore this and write what makes the most sense to you:

def joinMatchListWithCatalog(self, matchCat, sourceCat):

    """!Relink an unpersisted match list to sources and reference objects

    A match list is unpersisted as a catalog of IDs produced by afw.table.packMatches(),

    with match metadata (as returned by the astrometry tasks) in the catalog's metadata attribute.

    This method converts such a match catalog into a match list (an lsst.afw.table.ReferenceMatchVector)

    with links to source records and reference object records.

    @param[in]   matchCat       Unpersisted match list (an lsst.afw.table.BaseCatalog).

                                packedMatches.table.getMetadata() must contain match metadata,

                                as returned by the astrometry tasks.

    @param[in,out] sourceCat    Source catalog (an lsst.afw.table.SourceCatalog).

                                As a side effect, the catalog will be sorted by ID.

    @return the match list (an lsst.afw.table.ReferenceMatchVector)

"""

Show

Russell Owen added a comment - 18/Dec/15 3:33 PM - edited This looks great. My one request is to improve the documentation for the moved method. It was already confusing (and not PEP-8 compliant and did not work with Doxygen). But it makes even less sense now, because some of the things it referred to were part of its old class. The following might be a reasonable start, but feel free to ignore this and write what makes the most sense to you: def joinMatchListWithCatalog(self, matchCat, sourceCat): """!Relink an unpersisted match list to sources and reference objects A match list is unpersisted as a catalog of IDs produced by afw.table.packMatches(), with match metadata (as returned by the astrometry tasks) in the catalog's metadata attribute. This method converts such a match catalog into a match list (an lsst.afw.table.ReferenceMatchVector) with links to source records and reference object records. @param[in] matchCat Unpersisted match list (an lsst.afw.table.BaseCatalog). packedMatches.table.getMetadata() must contain match metadata, as returned by the astrometry tasks. @param[in,out] sourceCat Source catalog (an lsst.afw.table.SourceCatalog). As a side effect, the catalog will be sorted by ID. @return the match list (an lsst.afw.table.ReferenceMatchVector) """

Russell Owen made changes - 18/Dec/15 3:37 PM

Status

In Review [ 10004 ]

Reviewed [ 10101 ]

Hide

Permalink

Lauren MacArthur added a comment - 18/Dec/15 3:55 PM

Excellent point, and thanks for writing the docs yourself! I've adopted what you wrote with a few minor edits. I've pushed tickets/DM-3633 branches and am re-running Jenkins (just to be on the safe side). I will push to master if it succeeds.

Show

Lauren MacArthur added a comment - 18/Dec/15 3:55 PM Excellent point, and thanks for writing the docs yourself! I've adopted what you wrote with a few minor edits. I've pushed tickets/ DM-3633 branches and am re-running Jenkins (just to be on the safe side). I will push to master if it succeeds.

Lauren MacArthur made changes - 18/Dec/15 4:55 PM

Resolution		Done [ 10000 ]
Status	Reviewed [ 10101 ]	Done [ 10002 ]

Hide

Permalink

John Swinbank added a comment - 21/Dec/15 10:24 AM

This is your regular reminder to update the release notes! https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+W16+release+notes

Show

John Swinbank added a comment - 21/Dec/15 10:24 AM This is your regular reminder to update the release notes! https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+W16+release+notes

Lauren MacArthur made changes - 23/Dec/15 3:34 PM

Remote Link