DM-53484: Add background_stats_flux_column config. #1202
Conversation
erinleighh
force-pushed
the
tickets/DM-53484
branch
from
ed64aa7 to
30f214b
Compare
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
Looks good, just some comments about naming things.
| dtype=str, | ||
| default="base_CircularApertureFlux_12_0_flux", | ||
| doc="Aperture flux used to generate bg_subtracted_skySource_flux_median " | ||
| "and bg_subtracted_skySource_flux_stdev." |
There was a problem hiding this comment.
I think this description is a bit too specific, and will become out of date if the metrics change. Maybe "used to generate post-subtracted background stats", since that's the phrasing DM-53369 used?
There was a problem hiding this comment.
Looking at the code, does it specifically need to be an aperture flux?
| "camera model from the obs_* package." | ||
| ) | ||
|
|
||
| ap_flux_column = pexConfig.Field( |
There was a problem hiding this comment.
Just to make sure, should I be reading this as "aperture" and not a reference to the AP pipeline?
Even so, a name like "background_stats_flux_column" (to go with the existing background_stats_ignored_pixel_masks) might avoid any expectations about being used for other things.
There was a problem hiding this comment.
Thank you! bg_stats_flux_column sounds much better.
There was a problem hiding this comment.
Sorry, I edited my post before I saw your reply.
erinleighh
force-pushed
the
tickets/DM-53484
branch
from
30f214b to
8088a04
Compare
erinleighh
changed the title
erinleighh
force-pushed
the
tickets/DM-53484
branch
from
8088a04 to
214efb0
Compare
erinleighh
changed the title
No description provided.