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-30193: Add MakeVisitTableTask to postprocess.py #525
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.
Apologies for the drive-by review, but I noticed the quantities used were not self-consistent.
visitEntry["visitId"] = visitRow['visit'] | ||
visitEntry["filterName"] = visitRow['physical_filter'] | ||
visitEntry["ra"] = np.mean(visitSummary['ra']) | ||
visitEntry["decl"] = np.mean(visitSummary['decl']) |
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 position obtained from the mean of all the detectors is not going to be the same as the boresight position. I'm not sure which one we want, but we need to be consistent.
visitEntry["ra"] = np.mean(visitSummary['ra']) | ||
visitEntry["decl"] = np.mean(visitSummary['decl']) | ||
visitEntry["skyRotation"] = visitInfo.getBoresightRotAngle().asRadians() | ||
azimuth, altitude = visitInfo.getBoresightAzAlt() |
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 problem here is that the az/alt is going to be inconsistent with the ra/dec above, and that could cause much confusion. We need to make sure that they are consistent.
visitEntry["skyRotation"] = visitInfo.getBoresightRotAngle().asRadians() | ||
azimuth, altitude = visitInfo.getBoresightAzAlt() | ||
visitEntry["azimuth"] = azimuth.asRadians() | ||
visitEntry["altitude"] = altitude.asRadians() |
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.
All units in this table according to the baseline schema should be degrees, e.g. https://github.com/lsst/sdm_schemas/blob/master/yml/baselineSchema.yaml#L5527
visitEntry["azimuth"] = azimuth.asRadians() | ||
visitEntry["altitude"] = altitude.asRadians() | ||
visitEntry["zenithDistance"] = visitRow['zenithDistance'] | ||
visitEntry["airmass"] = visitInfo.getBoresightAirmass() |
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.
Note that the zenith distance in the visit summary table is computed at the center of each ccd. It is not the boresight zenith distance. I'm not sure which we want for this table (I personally don't know what "observation mid-point" means technically). But right now this code snippet will set the zenith distance to the first detector, and the airmass to the boresight. We need to ensure these are consistent.
Furthermore, "airmass" is a slippery quantity. There are many ways of computing airmass, and I don't know which one is supposed to be here. "zenithDistance" is unambiguous, thankfully.
a1c249b
to
c208d87
Compare
class MakeVisitTableConnections(pipeBase.PipelineTaskConnections, | ||
dimensions=("instrument",), | ||
defaultTemplates={}): | ||
visitSummaries = pipeBase.connectionTypes.Input( |
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 this just be connectionTypes
, instead of pipeBase.connectionTypes
?
c208d87
to
9355bb4
Compare
No description provided.