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-15073: draft Object.yaml #163
Conversation
policy/Object.yaml
Outdated
PsFlux: | ||
functor: NanoJansky | ||
filt: HSC-R | ||
# Should we include forced_src too? |
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.
If we have to pick one, I think including just the forced_src
flux would be more appropriate; the model described by the DPDD uses the same position (and motion) across all bands.
policy/Object.yaml
Outdated
# At the moment it only makes sense to do this per band | ||
# DPDD thinks this is an integer. It is not. | ||
functor: Column | ||
args: slot_PsfFlux_area |
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 the DPDD wants isn't just the footprint area, because the pixels we use are defined by the size of the PSF model bounding box, not the footprint. In any case, the larger point that it's not very meaningful when we're not doing a moving PS is certainly true.
However, base_PsfFlux_area
isn't really related to any of these (and that's why it's not an integer); it's a measure of PSF width that has units of area rather than units of length (and sadly, "effective area" means something totally different from "effective radius"). That's a useful quantity, but not one in the DPDD at all.
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 what the DPDD wants something that lets us go from Chi^2 to reduced Chi^2? I'm going to call psNdata
"Not computed yet,"
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, that's my understanding.
policy/Object.yaml
Outdated
- modelfit_CModel_exp_ellipse_xy | ||
- modelfit_CModel_exp_ellipse_yy | ||
#bdCov: Not computed | ||
#bdChi2: Not computed |
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.
modelfit_CModel_objective
is the chi^2.
In terms of location for this file, I don't think it needs to be hidden away in qa_explorer, but I also don't think it's at all HSC specific; it only seems that way because HSC data is what we regularly process through the prototype DRP pipelines, and I didn't see anything here I would expect to change if we were applying these functors to, say, DESC DC2 processing. I think obs_base, pipe_tasks, or a new package (presumably with other parquet-producing tasks) would be a better home. |
0571eb2
to
92ce508
Compare
There are explicit declarations of "HSC-R", "HSC-G" etc in here. We should be able to remove all references to filter names in the future and make the functors loop over filters even for colors in a smart way. For now, I don't want the letters "HSC" in that order to appear anywhere in |
dataset: ref | ||
coord_ra: | ||
# reference position for merged "ref" cat. Required by db. Not in DPDD | ||
functor: RAColumn |
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.
Should the “A” of RA really be capitalized?
Also, there’s a note about why coord_dec below, but no accompanying one for coord_ra here. Deliberate?
dataset: ref | ||
Ra: | ||
functor: RAColumn | ||
dataset: meas |
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.
Ditto re “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.
I don't a problem. RAColumn
is the name of Tim's Functo,r and the column names turn into gRa
, rRa
, iRa
as specified by cat and the dpdd.
policy/Object.yaml
Outdated
dataset: ref | ||
# psRadecTai: No moving point source model yet. | ||
# psRadec: No moving point source model yet. | ||
# psPm: No moving point source model yet. |
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.
Should the “d”s of dec be capitalized here?
policy/Object.yaml
Outdated
# DRP is working the the Bulge-Disk model fluxes | ||
# In the meantime, CModel is the best | ||
# CModel fluxes have alias slot_ModelFlux | ||
#bdRadec: # Same as SdssCentroid |
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.
Ditto re “d”
Space needed after #
Here and throughout, inconsistent use of period on comments.
policy/Object.yaml
Outdated
dataset: meas | ||
# Icov: # need to compute | ||
# DPDD should include Psf Shape: | ||
refIxxPSF: |
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.
“PSF” -> “Psf”? (and many cases below)
policy/Object.yaml
Outdated
FWHM: | ||
functor: Seeing | ||
dataset: meas | ||
refFWHM: |
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.
Should “FWHM” really be all capitals?
- slot_Shape_flag_no_pixels | ||
- slot_Shape_flag_not_contained | ||
- slot_Shape_flag_parent_source | ||
# Need to add per object Galactic extinction to post-processing |
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.
Sims is now installed with the shared stack, so could this be added using functions there (as is done in pipe_analysis
). It’s not part of lsst_distrib
, but then neither is qa_explorer
, yet... I’m not sure where the dependency boundaries lie...
# 1) PSF convolved undeblended aperture fluxes? | ||
# 2) Scarlet outputs? | ||
# In the meantime: forced CModel | ||
functor: Color |
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 this a Std from a given locus? Is the function to compute that really just called “Color”?
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 "Std" refers to "Standard Seeing" ls.st/LSE-163.
Yep: https://github.com/lsst-dm/qa_explorer/blob/master/python/lsst/qa/explorer/functors.py#L557
- modelfit_CModel_exp_ellipse_xy | ||
- modelfit_CModel_exp_ellipse_yy | ||
#bdCov: Not computed | ||
BdChi2: |
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.
“bd” vs “Bd”?
# photoZ: # Not computed yet | ||
|
||
# COLUMNS ADDED (not in the DPDD) | ||
x: |
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 the unit/frame here pixel/tract?
for use with TransformObjectCatalogTask. TransformObjectCatalogTask explodes columns per filter for all columns that are not from the dataset 'ref' or specified noDup
92ce508
to
3781009
Compare
I'm not attached to the file's location. I am amenable to moving it deeper or to qa_explorer for now