DM-5419

[r-owen]

After DM-4692, icSrc no longer has its coord fields set, because we
don't have a Wcs available before it's persisted. So when we later
match icSrc against the coadd catalog, we first have to populate the
coords.

[RobertLuptonTheGood]

I'm not happy about this. If we are still using icSrc why wasn't this done before the file was written? I understand that the Wcs solution has been moved later, but why couldn't we delay the write until after the coordinates were updated? I thought that the argument was that we weren't using icSrc, but now I see this ticket. Is this a temporary hack -- if so, it should have been called out as such

[TallJimbo]

The reason we can't easily delay the write is that we've now split ProcessCcdTask into two components which are both CmdLineTasks and hence handle their own I/O - this was in response to a feature request to allow them both to be run from the command-line separately. So if we delay the icSrc write until we have a Wcs, we'd have to make it the responsibility of a different command-line task, making the first one less self-contained and/or changing its I/O behavior based on whether or not it's being used as a subtask.

This is also in some respects a temporary change, because we'd like to be matching to src in the coadd processing, not icSrc, and that just requires us to turn on matching icSrc to src in ProcessCcdTask (I'm not sure why that hasn't been done yet, but I think it's related to both something not being merged from HSC and the ProcessCcdTask refactor).

All that said, I should say that I'm actually not at all bothered by this, because I think it might actually be better to not save celestial coordinates in any SourceCatalogs, for the same reason we save raw fluxes instead of calibrated magnitudes - things get confusing when the calibrations change. Obviously, having the coordinates there is also really convenient, so I'm not actually proposing that we remove them right now. But if we had some way to create coordinate columns on the fly, it'd be worth considering.

[r-owen]

On Mar 11, 2016, at 9:01 AM, Jim Bosch notifications@github.com wrote:

The reason we can't easily delay the write is that we've now split ProcessCcdTask into two components which are both CmdLineTasks and hence handle their own I/O - this was in response to a feature request to allow them both to be run from the command-line separately. So if we delay the icSrc write until we have a Wcs, we'd have to make it the responsibility of a different command-line task, making the first one less self-contained and/or changing its I/O behavior based on whether or not it's being used as a subtask.

This is also in some respects a temporary change, because we'd like to be matching to src in the coadd processing, not icSrc, and that just requires us to turn on matching icSrc to src in ProcessCcdTask (I'm not sure why that hasn't been done yet, but I think it's related to both something not being merged from HSC and the ProcessCcdTask refactor).

icSrc flags are already being copied to src flags in the new code, so I suggest you use the src catalog.

The only missing features I am aware of in the new ProcessCcdTask and subtasks, compared to specifications, are:

• No fake source injection. This is a lost feature. There is a ticket.
• IsrTask, CharacterizeImageTask and CalibrateTask cannot be run from the command line. This was a requested new feature. These are already command-line tasks but I haven’t made binary scripts to run them because I haven’t spent the time to add 6 new dataset types to every obs_* package (for the task config and metadata). Frankly I am hoping we can live without until we have butler dataset prototypes, or whatever they are officially called.

  All that said, I should say that I'm actually not at all bothered by this, because I think it might actually be better to not save celestial coordinates in any SourceCatalogs, for the same reason we save raw fluxes instead of calibrated magnitudes - things get confusing when the calibrations change. Obviously, having the coordinates there is also really convenient, so I'm not actually proposing that we remove them right now. But if we had some way to create coordinate columns on the fly, it'd be worth considering.

  I agree. That said, I suggest we offer a readily available function that sets the Coord field of a source catalog based on a WCS and the slot centroids. The code is simple, but why not make it readily available? And if speed becomes an issue we can rewrite it in C++. We already have production code doing this, as part of meas_astrom (and we have a function in there, but I’d rather see it in a more obvious location).

— Russell

[TallJimbo]

It looks like icSrc to src matching was actually enabled in the ProcessCcdTask refactor, so I've created DM-5424 to switch to using src here.

[TallJimbo]

That said, I suggest we offer a readily available function that sets the Coord field of a source catalog based on a WCS and the slot centroids.

Please create an issue. This would probably take me an hour at most, but it might also be a good starter mini-project for Pim. The tricky part is figuring out exactly where to put it, since SourceCatalog is just a typedef, but I have some ideas.