New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-21886: Create PipelineTask driver for ap_pipe tasks that interact with the APDB #68
Conversation
Naming changes for more consitencyy Debug association unittest. Convert to pandas in test. Fix linting.
Add docstring to diaPipe. Fix bugs. Debug diaPipe. Debug unittest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I had a few suggestions for clarity and some minor corrections.
diaSources. (`pandas.DataFrame`) | ||
- ``n_updated_dia_objects`` : Number of previously known dia_objects | ||
with newly associated DIASources. (`int`). | ||
- ``new_dia_oresultsbjects`` : Newly created DiaObjects from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "new_dia_oresultsbjects"
@@ -129,7 +129,7 @@ def __init__(self, **kwargs): | |||
self.makeSubtask("forcedMeasurement", | |||
refSchema=afwTable.SourceTable.makeMinimalSchema()) | |||
|
|||
def run(self, dia_objects, expIdBits, exposure, diffim, apdb): | |||
def run(self, dia_objects, expIdBits, exposure, diffim): | |||
"""Measure forced sources on the direct and different images, | |||
calibrate, and store them in the Apdb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this docstring need to be updated? run
no longer does anything with the apdb.
returnMaxBits=True) | ||
inputs["ccdExposureIdBits"] = expBits | ||
|
||
outputs = self.run(**inputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you just pass ccdExposureIdBits = expBits
directly, rather than packing it in inputs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly I did this parroting what was happening in calibration.py. They do a similar operation with the same value/variable.
Load previous DiaObjects and their DiaSource history. Calibrate the | ||
values in the diaSourceCat. Associate new DiaSources with previous | ||
DiaObjects. Run forced photometry at the updated DiaObject locations. | ||
Store the results in the Apdb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please spell out Apdb here.
import lsst.dax.apdb as daxApdb | ||
import lsst.pex.config as pexConfig | ||
import lsst.pipe.base as pipeBase | ||
import lsst.pipe.base.connectionTypes as connTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you spell out the full connectionTypes
(as is used in e.g. pipe_tasks) instead of shortening it to connTypes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I saw a few pipe_tasks that used cT
instead of connectionTypes
thought this was a good compromise?
make_dia_source_schema) | ||
|
||
__all__ = ("DiaPipelineConfig", | ||
"DiaPipelineTask") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need DiaPipelineConnections
in __all__
.
) | ||
diffIm = connTypes.Input( | ||
doc="Difference image on which the DiaSources were detected.", | ||
name="deepDiff_differenceExp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a template instead of hardcoding the "deep". I assume the Gen 3 differencing task, like the old one, will still have multiple types of diffs.
name="deepDiff_differenceExp", | |
name="{coaddName}Diff_differenceExp", |
Fix linting Fixup doc strings. Add DiaPipelineConnections to __all__
Add defaultTemplates to pipeline connections.
a07baa3
to
512872c
Compare
No description provided.