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

Update LoadReferenceObjectsTask to output fluxes in nanojansky

    XMLWordPrintable

    Details

    • Story Points:
      8
    • Sprint:
      AP S19-1, AP S19-2, AP S19-3, AP S19-4, AP S19-5
    • Team:
      Alert Production

      Description

      PhotoCalib is now defined in terms of nanojansky, but our reference catalogs are saved in Jy, and remain in Jy when the user loads them. We need to get our refcat loaders to produce nJy to keep our calibrations self-consistent.

      It may be enough to just multiply fluxes and flux errors by 1e9 after the data is read from disk, but Simon Krughoff may have other suggestions.

      We will have to add a few work-arounds for this to PhotoCal and other tasks that use the Calib object, as it will remained defined in Jy. If only we could do DM-10153 simultaneously (but I think that would be maddening).

        Attachments

          Issue Links

            Activity

            Hide
            krughoff Simon Krughoff added a comment -

            It seems cleaner to change the catalog ingesters to persist ref catalogs in nJy. I guess the argument against that is backward compatibility?

            I'd be ok if the conversion is done in the loader by simply multiplying by 1e9 as long as we document it clearly.

            Show
            krughoff Simon Krughoff added a comment - It seems cleaner to change the catalog ingesters to persist ref catalogs in nJy. I guess the argument against that is backward compatibility? I'd be ok if the conversion is done in the loader by simply multiplying by 1e9 as long as we document it clearly.
            Hide
            Parejkoj John Parejko added a comment -

            I think John Swinbank would insist that we maintain backwards compatibility with old refcats, but he should speak to that.

            Do existing refcats have their units preserved in a way that can be checked by the loader? My guess is not, so we'll have to version them somehow, with the existing unversioned one being "0", I suppose?

            Show
            Parejkoj John Parejko added a comment - I think John Swinbank would insist that we maintain backwards compatibility with old refcats, but he should speak to that. Do existing refcats have their units preserved in a way that can be checked by the loader? My guess is not, so we'll have to version them somehow, with the existing unversioned one being "0", I suppose?
            Hide
            price Paul Price added a comment -

            Wasn't there another recent breaking of reference catalog backwards compatibility with the proper motions? You might be able to sweep them up together if there's an official effort to regenerate the main reference catalogs people want.

            Show
            price Paul Price added a comment - Wasn't there another recent breaking of reference catalog backwards compatibility with the proper motions? You might be able to sweep them up together if there's an official effort to regenerate the main reference catalogs people want.
            Hide
            Parejkoj John Parejko added a comment -

            Jenkins run with ci_hsc: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29495/pipeline

            Jenkins passed (without ci_hsc), and without me making changes to meas_extensions_astrometryNet, which rather surprises me: I thought the demo used an a.net refcat?

            Show
            Parejkoj John Parejko added a comment - Jenkins run with ci_hsc: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29495/pipeline Jenkins passed (without ci_hsc), and without me making changes to meas_extensions_astrometryNet, which rather surprises me: I thought the demo used an a.net refcat?
            Hide
            Parejkoj John Parejko added a comment -

            Simon Krughoff: are you still ok to take on this large review? It turns out the changes outside of meas_algorithms were pretty minor.

            Suggestions for other ways to test this are welcome. I suspect some non-CI code will break due to the units change, but I don't have any way to test that, obviously. I would like to add deprecation warnings to the flux/mag conversion functions (preview of DM-16903), but we can't use deprecated yet, since it's not included in conda. If that changes before I merge, I will probably do so.

            pipe_tasks PR that wasn't picked up by Jira: https://github.com/lsst/pipe_tasks/pull/281/files

            Show
            Parejkoj John Parejko added a comment - Simon Krughoff : are you still ok to take on this large review? It turns out the changes outside of meas_algorithms were pretty minor. Suggestions for other ways to test this are welcome. I suspect some non-CI code will break due to the units change, but I don't have any way to test that, obviously. I would like to add deprecation warnings to the flux/mag conversion functions (preview of DM-16903 ), but we can't use deprecated yet, since it's not included in conda. If that changes before I merge, I will probably do so. pipe_tasks PR that wasn't picked up by Jira: https://github.com/lsst/pipe_tasks/pull/281/files
            Hide
            krughoff Simon Krughoff added a comment -

            I can take it. It's very weird that the pipe_tasks PR wasn't picked up.

            Show
            krughoff Simon Krughoff added a comment - I can take it. It's very weird that the pipe_tasks PR wasn't picked up.
            Hide
            Parejkoj John Parejko added a comment -

            pipe_tasks and afw PRs haven't been seen on any of my tickets for a long time. I poked DM-16840 about it...

            Show
            Parejkoj John Parejko added a comment - pipe_tasks and afw PRs haven't been seen on any of my tickets for a long time. I poked DM-16840 about it...
            Hide
            krughoff Simon Krughoff added a comment -

            I'm sorry John Parejko I reviewed all your PRs, but forgot to mark complete here.

            Show
            krughoff Simon Krughoff added a comment - I'm sorry John Parejko I reviewed all your PRs, but forgot to mark complete here.
            Hide
            Parejkoj John Parejko added a comment -

            I've incorporated your comments and added extra version checking and better version output for the convert refcat script.

            Post cleanup jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29520/pipeline

            I think I'm going to hold off on merging this until DM-10156 is ready to merge: I think those two should go in as part of the same weekly.

            Show
            Parejkoj John Parejko added a comment - I've incorporated your comments and added extra version checking and better version output for the convert refcat script. Post cleanup jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29520/pipeline I think I'm going to hold off on merging this until DM-10156 is ready to merge: I think those two should go in as part of the same weekly.
            Hide
            krughoff Simon Krughoff added a comment -

            Sounds good.

            Show
            krughoff Simon Krughoff added a comment - Sounds good.
            Hide
            jbosch Jim Bosch added a comment - - edited

            I've put a commit that adds the version to the reference catalog headers as well on u/jbosch/DM-17029 of meas_algorithms (you can also just cherry-pick 2157407684968b41fdf9615260b980b5754c2f9d).  All meas_algorithms tests pass (the version1 test data repo was updated on that commit), but I have not tested other packages; in general you'll need to re-run the upgrade script or re-ingest version 1 catalogs to get those to work.

            If you opt not to include that on this ticket, please let me know and I'll put it on a new one.  I would like to get it in before we start upgrading on-disk catalogs in earnest, though.

            Show
            jbosch Jim Bosch added a comment - - edited I've put a commit that adds the version to the reference catalog headers as well on u/jbosch/ DM-17029 of meas_algorithms (you can also just cherry-pick 2157407684968b41fdf9615260b980b5754c2f9d).  All meas_algorithms tests pass (the version1 test data repo was updated on that commit), but I have not tested other packages; in general you'll need to re-run the upgrade script or re-ingest version 1 catalogs to get those to work. If you opt not to include that on this ticket, please let me know and I'll put it on a new one.  I would like to get it in before we start upgrading on-disk catalogs in earnest, though.
            Show
            Parejkoj John Parejko added a comment - Post Jim Bosch gen3 additions and rebasing Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29603/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Thank you for the review Simon Krughoff, and for your gen3 butler updates, Jim Bosch.

            Merged and done (and now trying to merge DM-10156 on the same day!).

            Show
            Parejkoj John Parejko added a comment - Thank you for the review Simon Krughoff , and for your gen3 butler updates, Jim Bosch . Merged and done (and now trying to merge DM-10156 on the same day!).

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Simon Krughoff
              Watchers:
              Hsin-Fang Chiang, Jim Bosch, John Parejko, John Swinbank, Paul Price, Russell Owen, Simon Krughoff, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.