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-11422: Adapt prototype pipeline to use coadds as templates #10
Conversation
python/lsst/ap/pipe/ap_pipe.py
Outdated
(e.g., 'visit=101'). Defaults to 'visit=410929' if neither \ | ||
--templateId nor --templateRepo provided.") | ||
templateFlags.add_argument('-t', '--templateRepo', | ||
help="A URI to a Butler repository that will be searched for coadd templates") |
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 would say here that if templateRepo is provided templateId will be ignored (as in the code below)
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.
What are you suggesting I say, exactly? ArgumentParser
will already mark the two arguments as mutually exclusive in --help
.
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 ArgumentParser
stop execution if you specify both?
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.
Yes
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.
Okay, than I withdraw my concern here.
# - use a coadd as a template instead of a visit | ||
# dataId = 'visit=410985 ccdnum=25' # one g-band visit in Blind15A40 and one CCD for testing | ||
template = '410929' # one g-band visit in Blind15A40, temporarily hard-wired | ||
# Stringly typed code, but I don't see a safer way to do this in Python |
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.
Stringly, or strongly?
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.
Obviously the former.
python/lsst/ap/pipe/ap_pipe.py
Outdated
Metadata from any tasks run by this method | ||
''' | ||
log = lsst.log.Log.getLogger('ap.pipe._doIngestTemplates') | ||
if os.path.exists(os.path.join(templateRepo, 'deepCoadd')): |
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 make a TODO here that `deepCoadd' should be adjusted later: these are actually good seeing coadds, for instance.
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.
Will do, although there's a ticket open (DM-12557) to rework the short-circuiting system entirely.
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.
Is there a ticket for changing the directory? For example, are you planning to do it as part of DM-10978?
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 we're still waiting on an RFC (and implementation) of the coaddName
bit here: https://community.lsst.org/t/coaddition-tasks-redesign-rfd-request/2101/2
python/lsst/ap/pipe/ap_pipe.py
Outdated
|
||
|
||
def _doDiffIm(processed_repo, dataId, template, diffim_repo): | ||
def _doDiffIm(processed_repo, dataId, templateType, template, diffim_repo): | ||
''' | ||
Do difference imaging with a visit as a template and one or more as science |
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.
adjust the top-level docstring now that you can also use coadds as templates.
python/lsst/ap/pipe/ap_pipe.py
Outdated
TODO: allow 'template' to be either a visit ID or a repo name (DM-11422) | ||
If `templateType` is 'coadd', the input repository containing the | ||
template coadds. | ||
If `teplateType` is 'visit', the DECam data ID which will be used as a |
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
The system is designed so that old calls to ap_pipe will have the same behavior. The --templateRepo argument does not yet work and will probably raise a NameError.
This change is needed because ImageDifferenceTask assumes the templates are in the processed repository. We can have less hacky handling once ImageDifferenceTask supports an explicit location for templates.
The intended behavior for ap_verify is that it provides coadd templates as part of the Dataset framework, and can use its dataset management code to tell ap_pipe where to find the templates.
The CCD number was hardcoded into the visit-based template, for reasons unknown.
ab65a86
to
81a9584
Compare
This commit adds support for coadd templates to
ap_pipe
. Per a comment by @mrawls (old line 192),ap_pipe
retains the ability to use visit templates. The default behavior is unchanged when runningap_pipe
from the command line, but uses a coadd when running fromap_verify
.