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-33012 Update faro base class refcat loader to return single DataFrame #116
Conversation
@@ -126,6 +128,8 @@ def _getReferenceCatalog(self, butlerQC, dataIds, refCats, filterList, epoch=Non | |||
refCatCorrected : `numpy.ndarray` | |||
Catalog of reference objects with proper motions and color terms | |||
(optionally) applied. | |||
refCatFrame: pandas.dataframe |
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.
The returns have changed such that refCat
and refCatCorrected
are no longer being returned. In that case, these should be removed from the list of returns in the documentation. I think it is still useful to have the documentation that describes what is included in the merged reference catalog DataFrame, but maybe this information goes in the docstring after the one-line description and before the listing of Parameters.
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.
Agreed this docstring has been changed with a longer description at the beginning and the following returns
Returns
-------
refCat: pandas.dataframe
a reference catalog with original columns and corrected
coordinates (ra,dec) and reference magnitudes (refMag-/refMagErr)
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.
Would it make sense to change refCatFrame
-> refCat
since downstream users of the reference catalog will simply expect a DataFrame?
It also occurs to me that we may want to make sure there are no duplicate columns, and if there are probably supersede the catalog columns with the corrected columns. |
Yes, it would be a good idea to check for any duplicated columns. However, doesn't the |
Ah your are right, I had misremembered about adding the |
My inclination is return both at least for now (partly as a check that proper motions have been applied). In that case, we will need to be clear in the documentation about the meaning of (and distinction between) the two columns. |
applying a proper motion correction and reference magnitudes transformed to the lsst bandpass. | ||
|
||
returns a refCat with both the original loaded reference catalog and | ||
the coorected coordinates (ra,dec) and reference magnitudes (refMag-/refMagErr) |
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.
Suggest (refMag-/refMagErr-)
and also below just for consistency.
The doc string might want to use the words "color terms" when referring to the reference magnitudes to make clear that the fluxes of reference objects have been converted to the photometric system of the data that is being analyzed.
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.
Updated:
Loads the
lsst.afw.table.SimpleCatalog
reference catalog, computes ra and dec
(optionally) applying a proper motion correction. Also, color terms
are (optionally) applied to the reference magnitudes in order to transform
them to the data's photometric system.returns a refCat with both the original loaded reference catalog and
the coorected coordinates (ra,dec) and transformed reference magnitudes
(refMag-/refMagErr-)
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.
Thanks. Looks great. Two very minor comments.
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.
No additional comments, nice work on clarifying the docstring
14eadb4
to
57727cb
Compare
Previously the
_getReferenceCatalog
returned the loaded refcat and a corrected refcat.The corrected refcat had color trasformations applied, but had a nested list containing these magnitudes.
This caused downstream problems in using the
matchter_probablistic
task.The
_getReferenceCatalog
has been updated to return a single data frame that is a hstack of the refCat, and refCatCorrected.