DM-16299: Create placeholder AP DPDDifying task #31
Conversation
Add first tests for apDataProduct Fix bugs and run first apDataProduct unittest.
Debug all unittests for apDataProduct Add midPointTai value to minimal DiaSource schema. Add docs Move duplicated flux tests.
morriscb
force-pushed
the
tickets/DM-16299
branch
from
a86af21
to
caf32f9
Compare
| ---------- | ||
| inputCatalog: `lsst.afw.table.SourceCatalog` | ||
| Input catalog with data to be copied into new output catalog. | ||
|
|
There was a problem hiding this comment.
Add optional exposure to doc
| Output catalog with data copied from input and new column names. | ||
| """ | ||
| outputCatalog = afwTable.SourceCatalog(self.outputSchema) | ||
| outputCatalog.preallocate(len(inputCatalog)) |
There was a problem hiding this comment.
This doesn't look like the right way to do this. Have you seen:
https://github.com/lsst-dm/Oct15_bootcamp/blob/measurement/measurement/measurement.pdf?raw=true
Jim says: extend is more efficient than preallocate + for loop. (Also, in cases where the for loop is necessary, reserve is better than preallocate)
There was a problem hiding this comment.
Moved the first Task to the extend method and the DiaSource task to reserve. Thanks for sending this. Finding examples of how to use mappers within the python side of the stack is rather difficult.
There was a problem hiding this comment.
I learnt things!
(Not least that finding good documentation on afw::table is hard... :))
| @@ -0,0 +1,196 @@ | |||
| # | |||
There was a problem hiding this comment.
Did you forget to rename the module? Typically we name the files something similar to the Task it contains so that it's easy to find. I wouldn't necessarily know that apDataProductMaker.py contains APDataMapperTask. We also usually name our tasks with the verb first: so MapAPDataTask and the module mapApData.py
| outputRecord = outputCatalog.addNew() | ||
| outputRecord.assign(inputRecord, self.mapper) | ||
|
|
||
| if outputCatalog.isContiguous(): |
There was a problem hiding this comment.
In your testing, have you found that this is needed?
Jim says: "I also think the isContiguous check should be unnecessary; maybe best to turn that into an assert to make sure I'm not wrong about that and document-in-code that it's guaranteed."
Reviewer's personal preference is throwing exceptions instead of asserts, but coder's choice.
tests/test_ap_data_product_maker.py
Outdated
| import numpy as np | ||
| import unittest | ||
|
|
||
| from lsst.ap.association import \ |
There was a problem hiding this comment.
We discourage \ over
from lsst.ap.association import (APDataMapperConfig,
APDataMapperTask,
DiaSourceMapperConfig,
DiaSourceMapperTask)
tests/test_ap_data_product_maker.py
Outdated
| @@ -0,0 +1,234 @@ | |||
| # This file is part of ap_association. | |||
tests/test_ap_data_product_maker.py
Outdated
| self.exposure.getInfo().getVisitInfo().getDate().get( | ||
| system=dafBase.DateTime.MJD)) | ||
| for inputName, outputName in apDMapConfig.copyColumns.items(): | ||
| if inputName[:4] == "slot": |
There was a problem hiding this comment.
Also see python's inputName.startswith("slot")
| photoCalib = afwImage.PhotoCalib(1 / flux0, flux0Err / flux0 ** 2) | ||
|
|
||
| outputCatalog = afwTable.SourceCatalog(self.outputSchema) | ||
| outputCatalog.preallocate(len(inputCatalog)) |
There was a problem hiding this comment.
see reserve suggestion above.
Fix linting errors. Fix linting errors Add midPointTai to DiaSource value copies. Add exposure to run docstring. Rename files and tasks. Fix missed class renaming.
morriscb
force-pushed
the
tickets/DM-16299
branch
from
407ff33
to
2168508
Compare
No description provided.