Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-16650

Use nJy in PhotoCalib as the unit for calibrated fluxes

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None

      Description

      This implements RFC-549.  To my knowledge, PhotoCalib is currently the only part of the codebase that uses any unit for calibrated fluxes right now.  Please comment here if you know of any other code.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Calib has assumptions about Jansky baked into it (e.g. fluxFromABMag returns Jansky).

            Show
            Parejkoj John Parejko added a comment - Calib has assumptions about Jansky baked into it (e.g. fluxFromABMag returns Jansky).
            Hide
            Parejkoj John Parejko added a comment -

            I think we will have to think more carefully about how Calib and PhotoCalib interact as part of this ticket (or do the Cailb->PhotoCalib replacement first): jointcal assumes it trivially turn a Calib into a PhotoCalib, because Maggies is somewhat "generic". We'll have to be a bit more careful about units.

            Show
            Parejkoj John Parejko added a comment - I think we will have to think more carefully about how Calib and PhotoCalib interact as part of this ticket (or do the Cailb->PhotoCalib replacement first): jointcal assumes it trivially turn a Calib into a PhotoCalib, because Maggies is somewhat "generic". We'll have to be a bit more careful about units.
            Hide
            Parejkoj John Parejko added a comment -

            I'll see if I can do this before sprint planning happens...

            Show
            Parejkoj John Parejko added a comment - I'll see if I can do this before sprint planning happens...
            Show
            Parejkoj John Parejko added a comment - Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29128/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Eli Rykoff: do you think you can tackle this review? It's of order 500 lines, across 7 packages (the afw branch and PR aren't showing up in Jira, so I've linked it below).

            Here's the missing afw PR: https://github.com/lsst/afw/pull/420

            Jim Bosch: I've modified the meas_mosaic files in-place for now. I'm talking with Hsin-Fang Chiang about running this meas_mosaic branch on some PDR data and replacing the persisted files to check that that all works.

            Note that this change means that old PhotoCalibs cannot be read. I've filed DM-16830 to give versioning to PhotoCalib, but for this particular change we'd have to also version the underlying BoundedFields. Fortunately, PhotoCalib isn't in significant use right now, so this should not disrupt anything.

            Show
            Parejkoj John Parejko added a comment - Eli Rykoff : do you think you can tackle this review? It's of order 500 lines, across 7 packages (the afw branch and PR aren't showing up in Jira, so I've linked it below). Here's the missing afw PR: https://github.com/lsst/afw/pull/420 Jim Bosch : I've modified the meas_mosaic files in-place for now. I'm talking with Hsin-Fang Chiang about running this meas_mosaic branch on some PDR data and replacing the persisted files to check that that all works. Note that this change means that old PhotoCalibs cannot be read. I've filed DM-16830 to give versioning to PhotoCalib, but for this particular change we'd have to also version the underlying BoundedFields. Fortunately, PhotoCalib isn't in significant use right now, so this should not disrupt anything.
            Hide
            Parejkoj John Parejko added a comment -

            New Jenkins run, in the hopes that the macOS failure was due to it pulling code in the middle of a rebase: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29136/pipeline

            Show
            Parejkoj John Parejko added a comment - New Jenkins run, in the hopes that the macOS failure was due to it pulling code in the middle of a rebase: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29136/pipeline
            Hide
            erykoff Eli Rykoff added a comment -

            So I am looking through the updates, and the code that I've used, and noticed a couple places that might have been missed ( ? ):
            lsst.afw.image.fluxFromABMag(): https://github.com/lsst/afw/blob/master/src/image/Calib.cc
            lsst.pipe.tasks.PhotoCal: https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/photoCal.py

            Now this is part of the Great Calib/PhotoCalib divide, so maybe this is something on the ToDo list, but as it is there are parts of the code using Jy and parts using nJy.

            Show
            erykoff Eli Rykoff added a comment - So I am looking through the updates, and the code that I've used, and noticed a couple places that might have been missed ( ? ): lsst.afw.image.fluxFromABMag() : https://github.com/lsst/afw/blob/master/src/image/Calib.cc lsst.pipe.tasks.PhotoCal : https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/photoCal.py Now this is part of the Great Calib/PhotoCalib divide, so maybe this is something on the ToDo list, but as it is there are parts of the code using Jy and parts using nJy.
            Hide
            Parejkoj John Parejko added a comment -

            I'm not touching Calib as part of this work: it and PhotoCalib are currently used in different places, and when we replace Calib (which this ticket is an early step of), everything will be uniform "for free".

            None of our analysis code operates on the calibrated results, except for validate_drp, and that uses the magnitudes.

            Show
            Parejkoj John Parejko added a comment - I'm not touching Calib as part of this work: it and PhotoCalib are currently used in different places, and when we replace Calib (which this ticket is an early step of), everything will be uniform "for free". None of our analysis code operates on the calibrated results, except for validate_drp , and that uses the magnitudes.
            Hide
            erykoff Eli Rykoff added a comment -

            There's still the issue that right now one can call (from python) lsst.afw.image.fluxFromABMag() and this is something different from lsst::utils::ABMagnitudeToNanojansky() (though I guess this isn't exposed in Python by this name). The fact that this first call is actually part of Calib rather than PhotoCalib is not apparent to the user (for example me, until now) at all.

            Show
            erykoff Eli Rykoff added a comment - There's still the issue that right now one can call (from python) lsst.afw.image.fluxFromABMag() and this is something different from lsst::utils::ABMagnitudeToNanojansky() (though I guess this isn't exposed in Python by this name). The fact that this first call is actually part of Calib rather than PhotoCalib is not apparent to the user (for example me, until now) at all.
            Hide
            Parejkoj John Parejko added a comment -

            lsst.afw.image.fluxFromABMag() will be going away with Calib, so I'm not too worried about it (especially since it doesn't tell you what units come out of it).

            Show
            Parejkoj John Parejko added a comment - lsst.afw.image.fluxFromABMag() will be going away with Calib , so I'm not too worried about it (especially since it doesn't tell you what units come out of it).
            Show
            Parejkoj John Parejko added a comment - New Jenkins run with fix for abs : https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29142/pipeline
            Show
            Parejkoj John Parejko added a comment - - edited New post-review jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29187/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for your help with the review, Eli Rykoff! I posted in slack#dm-jointcal at the end of last week about the upcoming merge.

            Here is the Community post summarizing these changes: https://community.lsst.org/t/photocalib-is-now-defined-in-terms-of-nanojansky/3508

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thanks for your help with the review, Eli Rykoff ! I posted in slack#dm-jointcal at the end of last week about the upcoming merge. Here is the Community post summarizing these changes: https://community.lsst.org/t/photocalib-is-now-defined-in-terms-of-nanojansky/3508 Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Eli Rykoff
                Watchers:
                Eli Rykoff, Jim Bosch, John Parejko, Michael Wood-Vasey
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel