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

Rename "*_flux" fields to "*_instFlux" in SourceCatalogs

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      8
    • Sprint:
      AP F18-3, AP F18-4
    • Team:
      Alert Production

      Description

      This is the implementation ticket for RFC-322. The implementation is as follows:

      • Rename all of our *Flux_flux/*Flux_fluxSigma table fields to *Flux_instFlux*Flux_instFluxSigma to hold the post-ISR counts.
      • Add *Flux_flux and *Flux_mag fields for the post-calibrated flux (in Maggies) and magnitudes.
        *Rename the InstFlux slot to GaussianFlux, and remove the InstMag slot.
      • Add associated documentation to the above.
      • Pass these changes on to the relevant database groups to update e.g. cat.

      Implementing this will wait until Calib is removed from the stack and replaced by PhotoCalib (not yet scheduled, but likely within the next couple months).

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            we should do this immediately following on the changes in DM-15244.

            Show
            Parejkoj John Parejko added a comment - we should do this immediately following on the changes in DM-15244 .
            Hide
            wmwood-vasey Michael Wood-Vasey added a comment -

            I'd like to add explicitly listing updating the
            1. LSST Science Pipelines documentation
            2. Tutorials that we've run over the past couple of years

            This is implicitly covered by "Add associated documentation to the above.", but I'd just like to make it explicitly clear.

            Show
            wmwood-vasey Michael Wood-Vasey added a comment - I'd like to add explicitly listing updating the 1. LSST Science Pipelines documentation 2. Tutorials that we've run over the past couple of years This is implicitly covered by "Add associated documentation to the above.", but I'd just like to make it explicitly clear.
            Hide
            swinbank John Swinbank added a comment -

            Absolutely on (1).

            I'm worried that (2) isn't really actionable — I'm not aware of any centralized list of tutorials; I'm worried that we could start quibbling over the definition of “we”; and I'm worried that tutorials may well have bitrotted for other reasons unless they've been actively maintained. If there's a specific list of tutorials or other material that you'd like to request be updated, perhaps you could add that? (For generalised “you” — people other thatn Michael Wood-Vasey can also suggest material which should be updated).

            Show
            swinbank John Swinbank added a comment - Absolutely on (1). I'm worried that (2) isn't really actionable — I'm not aware of any centralized list of tutorials; I'm worried that we could start quibbling over the definition of “we”; and I'm worried that tutorials may well have bitrotted for other reasons unless they've been actively maintained. If there's a specific list of tutorials or other material that you'd like to request be updated, perhaps you could add that? (For generalised “you” — people other thatn Michael Wood-Vasey can also suggest material which should be updated).
            Hide
            wmwood-vasey Michael Wood-Vasey added a comment -

            John Swinbank Agree with your request to be specific!

            One could decide to freeze a specific demo and not guarantee that it still works and re-direct users to a different one.

            • There are some DMTN's that specifically list base_PsfFlux_flux: DMTN-006, DMTN-008. Updating these doesn't have to be part of the ticket, but might be appropriate.

            I understand know in principle things under lsst-dm aren't meant to be externally useful, but if we have such a seemingly usefully named repo we might choose to populate it with tutorials.

            Show
            wmwood-vasey Michael Wood-Vasey added a comment - John Swinbank Agree with your request to be specific! These are the tutorials I'm aware of: https://github.com/lsst-dm/tutorial-lsst2017 https://github.com/lsst-sqre/notebook-demo One could decide to freeze a specific demo and not guarantee that it still works and re-direct users to a different one. There are some DMTN's that specifically list base_PsfFlux_flux : DMTN-006, DMTN-008. Updating these doesn't have to be part of the ticket, but might be appropriate. A sidenote. Should we delete or re-purpose the following repo: https://github.com/lsst-dm/tutorials This hasn't been updated in 4 years. I understand know in principle things under lsst-dm aren't meant to be externally useful, but if we have such a seemingly usefully named repo we might choose to populate it with tutorials.
            Hide
            wmwood-vasey Michael Wood-Vasey added a comment -

            As a brief preview of implementing this ticket:

            44 results for: org:lsst-dm base_PsfFlux_flux
            https://github.com/search?utf8=%E2%9C%93&q=org%3Alsst-dm+base_PsfFlux_flux&type=Code

            35 results for: org:lsst base_PsfFlux_flux
            https://github.com/search?utf8=%E2%9C%93&q=org%3Alsst+base_PsfFlux_flux&type=Code

            Obviously there are other column names as well that will have to be updated. But this was fewer than I expected.

            Show
            wmwood-vasey Michael Wood-Vasey added a comment - As a brief preview of implementing this ticket: 44 results for: org:lsst-dm base_PsfFlux_flux https://github.com/search?utf8=%E2%9C%93&q=org%3Alsst-dm+base_PsfFlux_flux&type=Code 35 results for: org:lsst base_PsfFlux_flux https://github.com/search?utf8=%E2%9C%93&q=org%3Alsst+base_PsfFlux_flux&type=Code Obviously there are other column names as well that will have to be updated. But this was fewer than I expected.
            Hide
            Parejkoj John Parejko added a comment -

            Local rebuild made it all the way through lsst_distrib.

            First Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28538/pipeline

            Show
            Parejkoj John Parejko added a comment - Local rebuild made it all the way through lsst_distrib. First Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28538/pipeline
            Hide
            Parejkoj John Parejko added a comment - - edited

            The above found some problems running ci_hsc, which I think I've fixed locally.

            New Jenkins run:
            https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28571/pipeline

            Show
            Parejkoj John Parejko added a comment - - edited The above found some problems running ci_hsc, which I think I've fixed locally. New Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28571/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Russell Owen: I hope you don't mind me handing you this rather large review. It's no huge rush. Could you please mark the individual packages as Reviewed on github, so I can be sure that jira didn't miss any in its sidebar link (I think it found them all)?

            The commits should be broken up fairly sanely in the individual packages: I did some other cleanup along the way, so you can probably not review e.g. clang-format X.cc commits. It's up to you how carefully you want to review the rest. I didn't do bulk changes of "flux->instFlux" outside of afw, but rather targeted things that broke tests and/or were clearly incorrect, so some docstrings may have been missed.

            Show
            Parejkoj John Parejko added a comment - Russell Owen : I hope you don't mind me handing you this rather large review. It's no huge rush. Could you please mark the individual packages as Reviewed on github, so I can be sure that jira didn't miss any in its sidebar link (I think it found them all)? The commits should be broken up fairly sanely in the individual packages: I did some other cleanup along the way, so you can probably not review e.g. clang-format X.cc commits. It's up to you how carefully you want to review the rest. I didn't do bulk changes of "flux->instFlux" outside of afw, but rather targeted things that broke tests and/or were clearly incorrect, so some docstrings may have been missed.
            Hide
            Parejkoj John Parejko added a comment -

            A note I forgot to put in the review request: I put the flux->instFlux catalog read code into the version 2 block, instead of making a version 3. At the time it seemed appropriate, being just the week after you added version 2 (Sigma->Err), but now maybe it should be a different version? I don't know if it would mean we can't read files written with the versions in the last three weeks or not.

            Show
            Parejkoj John Parejko added a comment - A note I forgot to put in the review request: I put the flux->instFlux catalog read code into the version 2 block, instead of making a version 3. At the time it seemed appropriate, being just the week after you added version 2 (Sigma->Err), but now maybe it should be a different version? I don't know if it would mean we can't read files written with the versions in the last three weeks or not.
            Hide
            rowen Russell Owen added a comment -

            A few suggestions on the pull requests but overall this looks good. Thanks for the cleanups in afw and fixing the Travis file name in the stack demo.

            Show
            rowen Russell Owen added a comment - A few suggestions on the pull requests but overall this looks good. Thanks for the cleanups in afw and fixing the Travis file name in the stack demo.
            Show
            Parejkoj John Parejko added a comment - New post-review/rebase jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28652/pipeline
            Hide
            rowen Russell Owen added a comment -

            My guess is this should be version 3, especially since there were old "instFlux" fields (though by a different name?), but Jim Bosch may have a more informed option.

            Show
            rowen Russell Owen added a comment - My guess is this should be version 3, especially since there were old "instFlux" fields (though by a different name?), but Jim Bosch may have a more informed option.
            Hide
            jbosch Jim Bosch added a comment -

            If in doubt, increment the version.  Integers are cheap.

            Show
            jbosch Jim Bosch added a comment - If in doubt, increment the version.  Integers are cheap.
            Hide
            Parejkoj John Parejko added a comment - - edited
            Show
            Parejkoj John Parejko added a comment - - edited New run with schema version 3 defined: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28663/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            The work as described here is now merged, and I've made a Community post about it.

            Michael Wood-Vasey: I hear your concerns about documentation and tutorials. I'll take a look at the dev guide and other such docs tomorrow, and I'll see what I can do about the tutorials as well, but I make no promises about those or other jupyter notebooks. My changes should allow backward compatibility with the old schemas, so things should continue to at least work.

            Show
            Parejkoj John Parejko added a comment - The work as described here is now merged, and I've made a Community post about it. Michael Wood-Vasey : I hear your concerns about documentation and tutorials. I'll take a look at the dev guide and other such docs tomorrow, and I'll see what I can do about the tutorials as well, but I make no promises about those or other jupyter notebooks. My changes should allow backward compatibility with the old schemas, so things should continue to at least work.
            Hide
            wmwood-vasey Michael Wood-Vasey added a comment -

            :+1:

            Great, thanks John Parejko
            Community Post looks good and helpful.
            Thanks, everyone, for versioning the schema!

            Show
            wmwood-vasey Michael Wood-Vasey added a comment - :+1: Great, thanks John Parejko Community Post looks good and helpful. Thanks, everyone, for versioning the schema!

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Russell Owen
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Maria Patterson [X] (Inactive), Michael Wood-Vasey, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel