DM-47002: Add pixelScale and PSF model delta metrics to ConsDB schemas

[laurenam]

No description provided.

[JeremyMcCormick]

It seems as though imsim.yaml should have been updated and not apdb.yaml, as the ticket description mentions "LSSTComCam[Sim]" and not APDB.

If this is the case, please fix and leave the update as a separate commit from the ConsDB changes, mentioning that you are adding these fields to imsim in the commit message.

(Should I be mistaken here, please let me know, and I'll clear my review.)

[JeremyMcCormick]

See #266 (comment)

[laurenam]

Ah, sorry for the confusion. In working on this ticket I just happened to notice that apdb.yaml was missing the pixelScale entry. imsim.yaml already has it (as well as the two other metrics being added on this ticket), so does not require any update. I've split that change into a separate commit, but if it somehow adds a complication for this ticket, I can remove it and put it on another ticket.

And thanks for giving this a look!

[JeremyMcCormick]

Ah, sorry for the confusion. In working on this ticket I just happened to notice that apdb.yaml was missing the pixelScale entry. imsim.yaml already has it (as well as the two other metrics being added on this ticket), so does not require any update. I've split that change into a separate commit, but if it somehow adds a complication for this ticket, I can remove it and put it on another ticket.

And thanks for giving this a look!

Thank you for the clarification. I've cleared my review.

I'm going to have @andy-slac look at this just to confirm the APDB change.

[JeremyMcCormick]

I believe this could be arcsec/pixel here. (That is a valid unit.)

[laurenam]

Oh, really? When I first tried looking into this, it didn't seem to be...very happy to hear it is! I'll update this everywhere.

[JeremyMcCormick]

https://github.com/lsst/sdm_schemas/pull/266/files#r1809659673

[laurenam]

So here is it:
ivoa:unit: arcsec/pixel
(as opposed to fits:tunit: arcsec/pixel above. In truth, I have no idea what the criterion is for using one or the other, I'm just trying to follow the established pattern!)?

[JeremyMcCormick]

So here is it: ivoa:unit: arcsec/pixel (as opposed to fits:tunit: arcsec/pixel above. In truth, I have no idea what the criterion is for using one or the other, I'm just trying to follow the established pattern!)?

This is up to you really if you want to include these as units, but it is preferable to specify units using the appropriate fields if possible rather than putting this in the description.

Also, please use ivoa:unit rather than fits:tunit. At some point we'll be converting all the fits:tunit fields to ivoa:unit. (This is something someone did originally which was copied but we actually use IVOA units so it is more correct to use this field instead.)

[laurenam]

I used ivoa:unit for the cdb_*.yaml files, but fits:tunit for apdb.yaml since every other entry in that file uses it (we are trained for consistency within a file 😉).

[gpdf]

It's still on our list to unify the usage. For now I agree with your approach of keeping it consistent within a given file.

[JeremyMcCormick]

This looks okay from Data Engineering perspective.

You may also want to seek another review from a ConsDB systems expert as well who is more familiar with the columns being added.

[andy-slac]

I'm going to have @andy-slac look at this just to confirm the APDB change.

I don't think I'm qualified to comment on science content of apdb.yaml. If that column needs to be a part of DetectorVisitProcessingSummary, then certainly no objections from my side. Note that usually updates to APDB schema also require increment of the schema version (defined at the top of the file). DetectorVisitProcessingSummary is sort of special in a sense that we are not creating it yet. Still, I think it would be better to change version: "2.0.0" to version: "2.0.1" in case we'll need to do something special about it later.

[kadrlica]

@laurenam do we know enough to specify the units of the psf_ap quantities? I think flux will be in electrons and scaling by the psfSigma is electrons/pixel(?). I'm trying to work through some unit stuff for the lsst*.yml files on DM-47044.

[laurenam]

Heh, yeah, here's how I described them (https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/atools/calexpMetrics.py#L64-L65):

"psfApFluxDelta": "",  # This is a normalized-to-one value.
"psfApCorrSigmaScaledDelta": "",  # This is a multiplicative factor.

(the former is because it's a measurement of a PSF model, and I guess the latter should really add "scaled by psfSigma (which is in pixels)"). I couldn't come up with a precise unit, so figured it was best to leave them blank.

Thanks for taking this on!