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

Cleanup and document functors.py

    XMLWordPrintable

Details

    • 2
    • Ops Pipelines 2023
    • Data Release Production
    • No

    Description

      While working on DM-35939, I noticed some things in pipe_tasks/functors.py that should be cleaned up by someone who understands it (probably from DRP).

      • NanoMaggie: I think we should just remove this one: the project isn't going to produce any values in nanomaggies.
      • Mag/MagErr vs. Magnitude: I think we can safely just remove both of these, and `Photometry` and its subclasses. I think these are fully supplanted by the various `LocalPhotometry` classes.
        • Mag claims to take a `calib` object, which hasn't existed for years, and it calls calib.getFluxMag0 which hasn't existed on Calib's replacement PhotoCalib for years, either.
        • Magnitude (and Photometry and all it's subclasses) is undocumented, but also wants a `calib` object and calls `getFluxMag0`.
      • ConvertPixelToArcseconds and ConvertPixelSqToArcsecondsSq: I think the docstrings on these are reversed, and should probably be made into proper english.
      • SdssTraceSize: the docstring for this does not help me understand what an "sdss trace size" is.

      The following have no docstrings:

      • DeconvolvedMoments, HsmFwhm, E1, E2, RadiusFromQuadrupole, ReferenceBand, NumStarLabeller, StarGalaxyLabeller, Labeller, FootprintNPix, IDColumn

      Attachments

        Issue Links

          Activity

            I think bsmart has worked on ConvertPixelToArcseconds and ConvertPixelSqToArcsecondsSq but on a user branch. https://github.com/lsst/pipe_tasks/pull/788

            kannawad Arun Kannawadi added a comment - I think bsmart has worked on ConvertPixelToArcseconds and ConvertPixelSqToArcsecondsSq but on a user branch. https://github.com/lsst/pipe_tasks/pull/788

            I am just waiting on an okay to merge the docstring fix for arcsecond/arcsecondsq.

            bsmart Brianna Smart added a comment - I am just waiting on an okay to merge the docstring fix for arcsecond/arcsecondsq.

            On DM-21954, the following functors were removed altogether:

            • NanoMaggie
            • Magnitude and MagnitudeErr
            • NumStarLabeller
            • StarGalaxyLabeller (moved to qa_explorer)
            • Labeller (moved to qa_explorer)
            • FootprintNPix
            • IDColumn
            kannawad Arun Kannawadi added a comment - On DM-21954 , the following functors were removed altogether: NanoMaggie Magnitude and MagnitudeErr NumStarLabeller StarGalaxyLabeller (moved to qa_explorer) Labeller (moved to qa_explorer) FootprintNPix IDColumn
            lskelvin Lee Kelvin added a comment -

            Ticket progressed as a party-programming exercise with kannawad, yusra and dtaranu.

            PR: https://github.com/lsst/pipe_tasks/pull/811

            lskelvin Lee Kelvin added a comment - Ticket progressed as a party-programming exercise with kannawad , yusra and dtaranu . PR: https://github.com/lsst/pipe_tasks/pull/811
            lskelvin Lee Kelvin added a comment - - edited

            Thanks for reviewing dtaranu. Many lines have been touched in functors.py, with some doc-string clean up also occurring in postprocess.py. After internal discussion, no code was touched. We had considered removing the Mag and MagErr classes, but reverted that change following review of DM-21954 (PR) on which we noticed that the prior Magnitude/MagnitudeErr classes had already been removed.

            In summary therefore, only doc strings have been touched on this ticket, adding doc strings where they were missing previously and a large amount of dotting the i's and crossing the t's.

            PR: https://github.com/lsst/pipe_tasks/pull/811

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

            lskelvin Lee Kelvin added a comment - - edited Thanks for reviewing dtaranu . Many lines have been touched in functors.py , with some doc-string clean up also occurring in postprocess.py . After internal discussion, no code was touched. We had considered removing the Mag and MagErr classes, but reverted that change following review of DM-21954 ( PR ) on which we noticed that the prior Magnitude / MagnitudeErr classes had already been removed. In summary therefore, only doc strings have been touched on this ticket, adding doc strings where they were missing previously and a large amount of dotting the i's and crossing the t's. PR: https://github.com/lsst/pipe_tasks/pull/811 Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/39131/pipeline
            lskelvin Lee Kelvin added a comment -

            Further updates have been made following comments from tjenness and kannawad.

            First, the new "nitpick" mode in the latest documenteer package has been used to find and address all remaining doc-string concerns in the functors.py file:

            package-docs clean; \
            package-docs build -n 2>&1 | tee $WWW/pipe_tasks_build.txt; \
            more $WWW/pipe_tasks_build.txt | grep functors
            

            After many edits, this file now builds cleanly. I began making some small edits on other files, but soon realized that this effort is well beyond the scope of this ticket, so drew a line at addressing functors.py completely.

            Second, the Mag/MagErr classes have been retained as mentioned above. In an attempt to resolve the unused calib argument, these will now issue a FutureWarning if anyone should try to pass in a calib object. I have scheduled this argument for removal after v27 on DM-39914.

            As much as changed since the above edits, I've kicked off a further Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/39135/pipeline

            lskelvin Lee Kelvin added a comment - Further updates have been made following comments from tjenness and kannawad . First, the new "nitpick" mode in the latest documenteer package has been used to find and address all remaining doc-string concerns in the functors.py file: package-docs clean; \ package-docs build -n 2>&1 | tee $WWW/pipe_tasks_build.txt; \ more $WWW/pipe_tasks_build.txt | grep functors After many edits, this file now builds cleanly. I began making some small edits on other files, but soon realized that this effort is well beyond the scope of this ticket, so drew a line at addressing functors.py completely. Second, the Mag/MagErr classes have been retained as mentioned above. In an attempt to resolve the unused calib argument, these will now issue a FutureWarning if anyone should try to pass in a calib object. I have scheduled this argument for removal after v27 on DM-39914 . As much as changed since the above edits, I've kicked off a further Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/39135/pipeline
            dtaranu Dan Taranu added a comment -

            I made some minor suggestions, but it looks good to go after a rebase. Thanks.

            dtaranu Dan Taranu added a comment - I made some minor suggestions, but it looks good to go after a rebase. Thanks.
            lskelvin Lee Kelvin added a comment -

            Thanks Dan, much appreciated. Minor edits all merged, rebased. If you strongly feel that the two functors you identify do need to be renamed, I'd support your setting up a follow-up ticket to capture that effort. I agree that those name changes are probably beyond the scope of this ticket however.

            Branch merged and deleted, cheers.

            lskelvin Lee Kelvin added a comment - Thanks Dan, much appreciated. Minor edits all merged, rebased. If you strongly feel that the two functors you identify do need to be renamed, I'd support your setting up a follow-up ticket to capture that effort. I agree that those name changes are probably beyond the scope of this ticket however. Branch merged and deleted, cheers.

            People

              lskelvin Lee Kelvin
              Parejkoj John Parejko
              Dan Taranu
              Arun Kannawadi, Brianna Smart, Dan Taranu, Eli Rykoff, Eric Bellm, Ian Sullivan, John Parejko, Lee Kelvin, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.