Ticket #2055 (closed defect)

Opened 8 years ago

Last modified 8 years ago

afwTable aborts on record access

Reported by: becker Owned by: jbosch
Priority: blocker Milestone:
Component: afw Keywords:
Cc: rhl, rowen, bick, cloomis, price, jbosch Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:
import lsst.afw.table as afwTable
schema = afwTable.SourceTable.makeMinimalSchema()
table = afwTable.SourceTable.make(schema)
img = table.makeRecord()
img.table.getCentroidKey().getX()

Description

The following code aborts on afw 5.0.2.0+1.

Note I am trying to create a Source with an X,Y and that is it. Its not entirely clear how to do such a simple thing after the Source redesign.

Change History

comment:1 Changed 8 years ago by DefaultCC Plugin

  • Cc rhl, rowen, bick, cloomis, price, jbosch added

comment:2 Changed 8 years ago by becker

I don't see this anywhere in the docs (doxygen, source code, or Wiki), but it certainly should be made more apparent (if not easier) to create a table that contains a field for centroid; all the subfields had to be divined by trial and error...

    srcSchema   = afwTable.SourceTable.makeMinimalSchema()
    centroidKey = srcSchema.addField("centroid", type="Point<F8>")
    flagKey     = srcSchema.addField("centroid.flags", type="Flag")
    covKey      = srcSchema.addField("centroid.err", type="Cov<Point<F8>>")
    srcTable    = afwTable.SourceTable.make(srcSchema)
    srcTable.defineCentroid("centroid")
    for i in range(len(x)):
        src = srcTable.makeRecord()
        src.set(centroidKey.getX(), x[i])
        src.set(centroidKey.getY(), y[i])

comment:3 Changed 8 years ago by jbosch

This is actually much easier in C++ than in Python (see afw::table::addCentroidFields, defined in Source.h). I expected it would be used exclusively by the centroid algorithm classes in meas/algorithms, and it was a little tricky to SWIG up, so I didn't do that initially. But if there's a need to make measurement fields in other contexts I can SWIG it up. Do you need it just for unit tests?

comment:4 Changed 8 years ago by becker

In this case, I needed it for the TAN-SIP fit (matching sources with focal plane coords to sources with sky coords). But it seems like a common enough circumstance that it should at least be documented in a unit test, which is where I initially look for things like this.

comment:5 Changed 8 years ago by jbosch

  • Status changed from new to closed
  • Resolution set to fixed

This is now documented in the FAQ on the wiki:

http://dev.lsstcorp.org/trac/wiki/Winter2012/NewSourceFAQ

I'm closing the ticket as there's actually no code that needs to be modified (note that there is already an example in a unit test, in the setUp method in testSourceTable.py).

Sorry I took so long to close the loop on this.

comment:6 Changed 8 years ago by rhl

  • Status changed from closed to assigned
  • Resolution fixed deleted

I discussed this one with Jim. The error is one that reasonable people could make, and should therefore throw an exception not trigger an abort.

Please keep aborts (usually generated by failed assertions) to cases where the code is wrong, and that a casual user couldn't make the mistake. Jim could argue that this one's caused by a bug, but a bug that gets added to an FAQ doesn't count in my book!

comment:7 Changed 8 years ago by jbosch

  • Status changed from assigned to inTicketWork

comment:8 Changed 8 years ago by jbosch

  • Owner changed from jbosch to ktl
  • Status changed from inTicketWork to inStandardsReview
  • reviewstatus changed from notReady to needsReview

I have switched the assertions to exceptions. These checks occur for every record access, so if we notice performance issues we could consider moving them inside a #ifndef NDEBUG.

Now ready for review; here's the summary diff:

 include/lsst/afw/table/BaseRecord.h |   14 ++++++++++++--
 tests/testSimpleTable.py            |    4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)

comment:9 Changed 8 years ago by ktl

  • Owner changed from ktl to jbosch
  • Status changed from inStandardsReview to inTicketWork

Looks good to me. Go ahead and merge.

comment:10 Changed 8 years ago by jbosch

  • Status changed from inTicketWork to closed
  • reviewstatus changed from needsReview to reviewed
Note: See TracTickets for help on using tickets.