Conversation
yalsayyad
force-pushed
the
tickets/DM-54227
branch
4 times, most recently
from
15d54e5 to
8f0f3ab
Compare
natelust
left a comment
natelust
left a comment
There was a problem hiding this comment.
I made a few comments on a per-task basis, but in general if you make changes it would apply to the various permutations of bands as well.
| connections.inputCoadd: pretty_coadd | ||
| connections.outputCoadd: pretty_coadd_fixed_stars | ||
| python: | | ||
| config.brightnessThresh = 0.3*16500 |
There was a problem hiding this comment.
Why did you take this away from being a parameter? Parameters still work within a config file.
| description: | | ||
| A step to generate final data product | ||
| single band HiPS maps for data release productions | ||
| that may be run after deep_coadds are available. |
There was a problem hiding this comment.
I think we talked about how the images would be a first class data product as well, so it is not just the hips that are produced here, but also the images we would be distributing
| - makeMetricTableObjectTableCoreRefCatMatch | ||
| - objectTableCoreRefCatMatchWholeSkyPlot | ||
| - step4e-measure-variability-hips | ||
| - step3c-coadd-hips |
There was a problem hiding this comment.
wait we are still making modifications to the FL pipeline? Because that has a different set of configurations to what was done here.
There was a problem hiding this comment.
I'm excluding the new task so that I don't make a change to DRP-FL.
| connections.inputCoadds: pretty_coadd_fixed_stars | ||
| connections.outputRGB: pretty_coadd_fixed_stars_gri | ||
| connections.outputRGBMask: pretty_coadd_fixed_stars_gri_mask | ||
| makeHighOrderHipsEpoGRI: |
There was a problem hiding this comment.
we should double check that epo only wants GRI (but maybe you already did)
| config: | ||
| # Task default is gri | ||
| connections.inputCoadds: pretty_coadd_fixed_stars | ||
| connections.outputRGB: pretty_coadd_fixed_stars_gri |
There was a problem hiding this comment.
These names make it seem like they come from the fixed star task rather than the image task. I'm not sure fixed_stars is needed in this name (and elsewhere), or something other than just bands added to the name. Remember this is going to be the public name that people consume when loading the color images in the data release, its not just internal.
| class: lsst.pipe.tasks.prettyPictureMaker.PrettyPictureTask | ||
| config: | ||
| connections.inputCoadds: deep_coadd | ||
| connections.outputRGB: pretty_deep_coadd_u |
There was a problem hiding this comment.
I don't think we expect these "images" to be distributed in the same way as data products as the color images, but if we do think about the names here, we probably want some common system to help indicate them to users.
yalsayyad
force-pushed
the
tickets/DM-54227
branch
from
a7ea6b5 to
8633c09
Compare
| config: | ||
| # Task default is gri | ||
| connections.inputCoadds: pretty_coadd_fixed_stars | ||
| connections.outputRGB: pretty_coadd_gri |
There was a problem hiding this comment.
If it's not too late to append _array or something to this dataset type name, that would make the transition away from a bare numpy array easier in a week or two.
(Same for all instances of this connection.)
| # Task default is gri | ||
| connections.inputCoadds: pretty_coadd_fixed_stars | ||
| connections.outputRGB: pretty_coadd_gri | ||
| connections.outputRGBMask: pretty_coadd_gri_mask |
There was a problem hiding this comment.
@natelust , does this imply that I should be adding a mask plane to lsst.images.ColorImage over on DM-54220?
There was a problem hiding this comment.
I added these in here originally to future proof things. The idea is that we may want the mask information for additional processing that might need done on the images (for instance it fuses the mask info of the 3 bands).
To this point we have not used it, but I am not quite ready to abandon it yet either. However, it is 100% supposed to be a pipeline implementation thing for processing, and is not intended to communicate anything to users or be used after the completion of images. If anyone can think of a compelling counter to that I am open to hearing it.
It is also currently saved as an afw.image.Mask object, so not a bare array. As far as keeping the state together, I think relying on the butler to associate the different data products together is probably fine for this.
natelust
force-pushed
the
tickets/DM-54227
branch
2 times, most recently
from
0555789 to
adb7eb0
Compare
leeskelvin
changed the title
leeskelvin
left a comment
leeskelvin
left a comment
There was a problem hiding this comment.
A couple more comments, but again, looking fairly healthy. As in ci_lsstcam, don't forget to clean up the squash/fixup commits here too.
| parameters: | ||
| base_uri: "" | ||
| hips_title: "" | ||
| abs_max: 75500 |
There was a problem hiding this comment.
Shouldn't this be a camera-specific override? Might be overkill, but I can see it being a potential gotcha when/if this is ever used on other cameras.
There was a problem hiding this comment.
possibly yes, but many of our pipelines are by default tuned to this is what is run an in these ways for the lsst drp processing. If we were to factor this out, we would need to add a whole other level of pipeline indirection because this is currently applied to all the pipelines we run things in, as this is a parameters override, not just a task config override for one camera. In principle these can be handled but in the spirit of keeping changes as minimal as possible for dp2 back porting late in the game I think it is best as is.
natelust
force-pushed
the
tickets/DM-54227
branch
from
adb7eb0 to
ff9ce64
Compare
Include the pretty picture and new hips tasks in the drp pipelines. This includes making pretty pictures for pretty_coadd for public consumption as well as outputs for standar coadd processing. Individual gray scale hips maps are also produced.
natelust
force-pushed
the
tickets/DM-54227
branch
from
ff9ce64 to
098c2f3
Compare
4 participants
No description provided.