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-8828: Support proper motions in reference catalogs #82
Conversation
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. My one suggestion is to pass epoch
around as a DateTime
python/lsst/meas/astrom/ref_match.py
Outdated
""" | ||
exposureInfo = exposure.getInfo() | ||
filterName = exposureInfo.getFilter().getName() or None | ||
if filterName == "_unknown_": | ||
filterName = None | ||
epoch = None | ||
if exposure.getInfo().hasVisitInfo(): | ||
epoch = exposure.getInfo().getVisitInfo().getDate().get() |
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.
Here is another place where returning a DateTime
instead of a float would, I think, be an improvement.
@@ -92,22 +92,23 @@ def __init__(self, butler=None, refObjLoader=None, **kwargs): | |||
self.makeSubtask("sourceSelection") | |||
self.makeSubtask("referenceSelection") | |||
|
|||
def run(self, catalog, filterName=None): | |||
def run(self, catalog, filterName=None, epoch=None): |
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.
Pease accept epoch
as a DateTime
instead of a float in MJD TAI, in order to avoid an obvious source of errors (incorrect system or scale).
""" | ||
exposureInfo = exposure.getInfo() | ||
filterName = exposureInfo.getFilter().getName() or None | ||
if filterName == "_unknown_": | ||
filterName = None | ||
epoch = None | ||
if exposure.getInfo().hasVisitInfo(): |
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.
Why would an exposure not have VisitInfo?
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.
It is easy to make an exposure without VisitInfo so it is not safe to assume there is one. That said, it is more likely to happen for unit test than in real life so I suppose we could raise an exception at this point.
feb3c39
to
dfeaf43
Compare
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.
LGTM, modulo other comments
976dce5
to
be05087
Compare
Mainly this consists of getting the epoch (TAI MJD) of the exposure into the catalog loader.
be05087
to
60c257d
Compare
Mainly this consists of getting the epoch (TAI MJD) of the exposure into
the catalog loader.