Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-901

Deprecate most input-calibration dispatch connections and configs in favor of finalVisitSummary

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Adopted
    • Resolution: Unresolved
    • Component/s: DM
    • Labels:
      None

      Description

      DM-35207 will shortly be adding a task (updateVisitSummary) that produces a new finalVisitSummary dataset that I think can be used to simplify a lot of downstream task and pipeline code.

      Like the existing visitSummary dataset, finalVisitSummary is a per-visit ExposureCatalog with a row for each detector, columns with metadata and summary statistics, and object columns that hold various exposure components like the SkyWcs, Psf, ApCorrMap, and PhotoCalib. The new task is intended to be run just before coaddition, and hence after all of those objects are in their final forms - whatever that means for any particular pipeline (e.g. HSC pipelines typically run jointcal and FGCM, while DECam pipelines just use jointcal at present, and ImSim doesn't run either).

      As a result, any task that wants the final forms of any of these could just use a single finalVisitSummary connection to obtain any number of them. And that's what I've done on DM-35207 already - since this can be dropped into all of the existing "global" SkyWcs and PhotoCalib connections we have, I've made finalVisitSummary the new default for all of those, and toggled the global/tract config flags to make them the ones used. This already makes the ExposureCatalog outputs that are produced directly by Jointcal, FGCM, and finalizeCharacterization into short-duration temporaries (we can delete them all sometime in step2 in all pipelines I've surveyed). It also makes our pipelines simpler, because they have more common with each other.

      This RFC is about going a step further: deprecating existing connections and configurations that select different exposure-component input catalogs in favor of (in most cases) a single finalVisitSummary input. This reduces complexity (good) and flexibility (maybe bad), but I think it's flexibility we don't need (that's the main point I'm interested in requesting comments about). It also has the potential to reduce I/O, which may be significant for some I/O-bound tasks.

      Details

      MakeWarpTask and CoaddBaseTask

      From MakeWarpTask, deprecate all external* connections, the finalizedPsfApCorrCatalog connection, and the wcsList and bboxList connections.

      Deprecate all external config fields ; some of these are already deprecated as Gen2, and all of them should have only ever been on MakeWarpTask, not other CoaddBaseTask subclasses.

      MakeWarpTask already has a visitSummary connection that can replace all of these, and I believe it does not need to have a config option to make it get exposure components directly from the calexp instead: it is cleaner to run updateVisitSummary in a mode that just passes those objects through (as it currently does in ImSim pipelines).

       WriteRecalibratedSourceTableTask

      Deprecate all external* connections and related configs.

      Add a new visit summary input connection to replace these.

      Again, I do not expect to need a config toggle for using the calexp components; we can always pass those through a visit summary dataset.

      ProcessCcdWithFakes and ImageDifferenceTask

      These tasks are already deprecated in their entirety, or will soon be. It's not worth touching them.

      See later comment (Jira refuses to link to it here).

      ForcedPhotCcdTask

      Again deprecate connections and config fields that control them.

      Add a new visit summary input connection.

      Again I do not think a config toggle for using calexp components is worth it, because we could pass those through visit summaries instead. Here I am less certain; given that it uses a skymap-based forcing catalog, I think this is a DRP-only task, and AP forced photometry would require a task with a different set of connections (at least) - but I'd like to have that assertion checked by the AP team. I don't think we want AP to have to run visit summaries, at least not with low latency, as they're a full-focal-plane sequence point.

      GetTemplateTask

      This doesn't have connections that take DRP-style final calibrations at all, just a TODO comment for DM-31292, but when that ticket is implemented, it should do so by adding a visit summary input connection as well as a config toggle to select between that and the existing "wcs" connection.

      SubtractImagesTask

      Deprecate the finalizedPsfApCorrCatalog connection and add a new visit summary one. This is just a rename, though it leaves the door open to using updated PhotoCalib and WCS in the future.

      Similarly deprecate the doApplyFinalizedPsf and add a new doApplyFinalCalibrations (or similar) replacement, leaving room for using more than the PSF instead of implying that more config toggles should be added (I maintain that these tasks are either run in AP mode without any visit summaries or in DRP mode with all components coming from visit summaries; I can't think of anything in between).

      faro (DetectorMeasurement and MatchedCatalogBase).

      As in most cases above, deprecate connections and related configs. Add a visit summary input.

      (Links are to DetectorMeasurement, but MatchedCatalogBase has identical blocks).

      Here I am again less certain about whether we should have a toggle that lets these use the calexp inputs for AP usage. The DM-35207 changes actually already assume we don't need that, since I think these tasks are ultimately going to be replaced before they're used in the AP pipeline and I don't see any of them in the AP pipelines now. But I'd very much like to have that confirmed (Jeffrey Carlin?). I do not see any pipelines in drp_pipe, ap_pipe, or ap_verify that would need to use them, since they are only being included in pipelines with visit summaries already present.

      What have I missed?

      Are there any other tasks that should be similarly updated? DM-35207 will already replace some usage of the initial visitSummary with the finalVisitSummary in analysis_tools, analysis_drp, and HealSparsePropertyMapTask, but I believe those are really about the summary stats, not the exposure components. And I think all new analysis code that wants calibrated sources should do so via a calibrated-source dataset, not by reapplying the calibration objects from a visit summary to a source catalog itself. Are there any exceptions to that?

      A comment below (Jira refuses to link to it) includes some later additions to this RFC's proposals; some changes have already been annotated above.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            I thought visitSummary and finalVisitSummary were already substitutable (wasn't that the point of the recent tickets?)

            They're almost substitutable, but finalVisitSummary has PSFs while visitSummary does not (at present).  This didn't affect the current ticket because the only task that currently consumes those PSFs is MakeWarp, and in all current pipelines that is either configured to get them from finalVisitSummary or not get them from a visit summary of any kind.  Many more tasks consume the final WCS and/or PhotoCalib, and for those (and many other things) visitSummary and finalVisitSummary are already substitutable.

            Show
            jbosch Jim Bosch added a comment - I thought visitSummary and finalVisitSummary were already substitutable (wasn't that the point of the recent tickets?) They're almost substitutable, but finalVisitSummary has PSFs while visitSummary does not (at present).  This didn't affect the current ticket because the only task that currently consumes those PSFs is MakeWarp, and in all current pipelines that is either configured to get them from finalVisitSummary or not get them from a visit summary of any kind.  Many more tasks consume the final WCS and/or PhotoCalib, and for those (and many other things) visitSummary and finalVisitSummary are already substitutable.
            Hide
            Parejkoj John Parejko added a comment - - edited

            I don't think it affects this RFC as it stands, but I should note here that we will want to use jointcal/gbdes WCS and fgcmcal PhotoCalib to construct our coadds for AP. We're not doing that currently because either we don't have enough data (for e.g. ap_verify datasets) or a Task doesn't work (e.g. fgcmcal on DECam), but we definitely will want the best wcs/photoCalib/psf when constructing coadds for AP templates for LSSTCam data.

            This may mean using a tweaked DRP pipeline to construct those coadds in year 1, which I think is ok. After DR1, I hope that we can just use the previous year's DRP coadds.

            Show
            Parejkoj John Parejko added a comment - - edited I don't think it affects this RFC as it stands, but I should note here that we will want to use jointcal/gbdes WCS and fgcmcal PhotoCalib to construct our coadds for AP. We're not doing that currently because either we don't have enough data (for e.g. ap_verify datasets) or a Task doesn't work (e.g. fgcmcal on DECam), but we definitely will want the best wcs/photoCalib/psf when constructing coadds for AP templates for LSSTCam data. This may mean using a tweaked DRP pipeline to construct those coadds in year 1, which I think is ok. After DR1, I hope that we can just use the previous year's DRP coadds.
            Hide
            tjenness Tim Jenness added a comment -

            Can we mark this RFC as implemented?

            Show
            tjenness Tim Jenness added a comment - Can we mark this RFC as implemented?
            Hide
            jbosch Jim Bosch added a comment -

            Deprecations are in, removals are not. I've been tending to leave RFCs open until removals are done, too, but I don't know if that's correct. I see that I didn't link the tickets consistently with that in this case.

            Show
            jbosch Jim Bosch added a comment - Deprecations are in, removals are not. I've been tending to leave RFCs open until removals are done, too, but I don't know if that's correct. I see that I didn't link the tickets consistently with that in this case.
            Hide
            tjenness Tim Jenness added a comment -

            General policy is that if there are no open triggered tickets for an RFC then all work is complete. ie I will move that "relates to" removal ticket to "is triggering".

            Show
            tjenness Tim Jenness added a comment - General policy is that if there are no open triggered tickets for an RFC then all work is complete. ie I will move that "relates to" removal ticket to "is triggering".

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Watchers:
              Ian Sullivan, Jim Bosch, John Parejko, Krzysztof Findeisen, Lee Kelvin, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Planned End:

                  Jenkins

                  No builds found.