DM-11807: Create initial DIAObject summary statistics for ap_verify #7
Conversation
Created additional summary statistic calculation for this column. Fixed bug in _store_n_associated_sources. Code current passes all unittests.
Added additional test to handle this case of differing schemas. Fixed bug in typing of schema for test_dia_source_append_different_schema
| self._dia_source_catalog.append( | ||
| self._dia_source_catalog.getTable().copyRecord( | ||
| input_dia_source_record)) | ||
| # We need to test that schema of the source to appended lines up with |
| input_dia_source_record)) | ||
| else: | ||
| tmp_source_record = afwTable.SourceTable.makeRecord( | ||
| afwTable.SourceTable.make(self._dia_source_schema)) |
There was a problem hiding this comment.
Why is this makeRecord(make())? Can't you just pass the schema to makeRecord?
There was a problem hiding this comment.
You are correct. Since I have a SourceCatalog available I can use the makeRecord method on that to create the empty record object. If I didn't have the catalog sitting around and wanted to create a SourceRecord I would need to first create the SourceTable from the schema which is what this line does.
| # assuming that the schema defined in this dia_object is a subset of | ||
| # of the input source's schema. | ||
| input_schema = input_dia_source_record.getSchema() | ||
| if input_schema == self._dia_source_schema: |
There was a problem hiding this comment.
This if:else: block seems overly complicated. Is there not a simpler way to add the record?
There was a problem hiding this comment.
Its complicated because I want to get a copy of the record as well as make sure the schemas line up. The way to do the second could possibly be taken care of by a SchemaMapper but creating the mapping may in fact result in more complex code than what you see here.
tests/test_dia_object.py
Outdated
| @@ -42,6 +42,9 @@ def create_test_dia_sources(n_sources=5): | |||
| ------- | |||
| A lsst.afw.SourceCatalog | |||
| """ | |||
| if schema is None: | |||
| schema = schema | |||
There was a problem hiding this comment.
What's going on here? This line makes no sense.
There was a problem hiding this comment.
Oops, I wanted this to be make_minimal_dia_source_schema(). It is now fixed.
tests/test_dia_object.py
Outdated
| @@ -144,6 +147,40 @@ def test_dia_source_append_and_update(self): | |||
| dia_obj.update() | |||
| self._compare_dia_object_values(dia_obj, 0, compare_values) | |||
|
|
|||
| def test_dia_source_append_different_schema(self): | |||
There was a problem hiding this comment.
Does the above test test the "same schema" case, or do you need a different test for that?
There was a problem hiding this comment.
It does test the same schema case.
tests/test_dia_object.py
Outdated
|
|
||
| compare_values = { | ||
| "coord_ra": 0.4999619199226218, | ||
| "coord_dec": 0.5000190382261059 |
There was a problem hiding this comment.
Where do these numbers come from?
There was a problem hiding this comment.
I've added a comment. Basically it's just average position of the incremented DIASources that are put into this object.
There was a problem hiding this comment.
It would be better if you actually computed that average: these are too much "magic value" for my tastes, given that they depend on code that lives elsewhere in this file.
There was a problem hiding this comment.
Okay, I'll use the original source catalog arrays to compute the values to get rid the magic numbers.
| """ | ||
| single_source = create_test_dia_sources(1) | ||
| dia_obj = DIAObject(single_source, None) | ||
| self.assertEqual(dia_obj.id, single_source[0].getId()) |
There was a problem hiding this comment.
Can you do self.assertEqual(dia_obj, single_source[0]) to test all the fields?
There was a problem hiding this comment.
No. I'm testing here that some of the more important values have been copied into the dia_object which is not the same data structure as the source record I access from the source catalog.
|
Committed first round of responses to review. |
Removed magic numbers from dia_object unittests. Fixed bug in unittests. Fixed bug in dia_object append method.
morriscb
force-pushed
the
tickets/DM-11807
branch
from
d24364b
to
7bed99c
Compare
No description provided.