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

Add analysis_drp to lsst_distrib

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      analysis_drp is a gen 3 only plotting package for QA purposes. It will hopefully replace pipe_analysis in serving DM's QA plotting needs.

        Attachments

        1. colour_colour.png
          colour_colour.png
          248 kB
        2. scatter.png
          scatter.png
          125 kB
        3. skyPlot.png
          skyPlot.png
          270 kB

          Issue Links

            Activity

            No builds found.
            sophiereed Sophie Reed created issue -
            tjenness Tim Jenness made changes -
            Field Original Value New Value
            Status Proposed [ 10805 ] Flagged [ 10606 ]
            Hide
            jbosch Jim Bosch added a comment - - edited

            Big from me.  pipe_analysis was never added to lsst_distrib in large part because we never got it conformant with DM's coding standards and workflow, while analysis_drp was designed to be conformant from the beginning.  We want to start running some of its PipelineTasks in our production pipelines, including the ones for DP0.2.

            Show
            jbosch Jim Bosch added a comment - - edited Big from me.  pipe_analysis was never added to lsst_distrib in large part because we never got it conformant with DM's coding standards and workflow, while analysis_drp was designed to be conformant from the beginning.  We want to start running some of its PipelineTasks in our production pipelines, including the ones for DP0.2.
            sophiereed Sophie Reed made changes -
            Attachment skyPlot.png [ 53555 ]
            Attachment scatter.png [ 53556 ]
            Attachment colour_colour.png [ 53557 ]
            Hide
            sophiereed Sophie Reed added a comment -

            Included are some examples of the plots that code from analysis_drp makes. There are more plot types being developed.

            Show
            sophiereed Sophie Reed added a comment - Included are some examples of the plots that code from analysis_drp makes. There are more plot types being developed.
            Hide
            lskelvin Lee Kelvin added a comment -

            I fully support this RFC. As someone who has been using analysis_drp for a while now, and beginning to develop plots of my own using analysis_drp, I find this resource to be incredibly valuable.

            Show
            lskelvin Lee Kelvin added a comment - I fully support this RFC. As someone who has been using analysis_drp for a while now, and beginning to develop plots of my own using analysis_drp , I find this resource to be incredibly valuable.
            Hide
            ktl Kian-Tat Lim added a comment - - edited

            The table file should have astropy, matplotlib, numpy, and scipy removed.  If this is Gen3 only, how come there is still a dependency on daf_persistence?  Should there be a direct dependency on daf_butler (rather than only indirect through pipe_base)? Can't qa_explorer be removed, as I don't see any use of its code?

            Show
            ktl Kian-Tat Lim added a comment - - edited The table file should have astropy , matplotlib , numpy , and scipy removed.  If this is Gen3 only, how come there is still a dependency on daf_persistence ?  Should there be a direct dependency on daf_butler (rather than only indirect through pipe_base )? Can't qa_explorer be removed, as I don't see any use of its code?
            Hide
            Parejkoj John Parejko added a comment -

            You should probably remove the examples path: I don't think that serves a purpose in this package, most of our older examples have bitrotted because they aren't run, and the folder is empty here anyway. Similarly for bin.src, since it's empty, too.

            I'll note that the tests directory is also empty: I realize that this is mostly plotting code, but there is also some analytic code that really should have tests.

            Show
            Parejkoj John Parejko added a comment - You should probably remove the examples path: I don't think that serves a purpose in this package, most of our older examples have bitrotted because they aren't run, and the folder is empty here anyway. Similarly for bin.src , since it's empty, too. I'll note that the tests directory is also empty: I realize that this is mostly plotting code, but there is also some analytic code that really should have tests.
            Hide
            mrawls Meredith Rawls added a comment -

            Yay! Please ensure the package has some docs and tests, and no extraneous directories or table file entries, as K-T and John point out. Looking forward to borrowing lots of what you've done for analysis_ap, so the more good life choices you can make here, the happier we will all be!

            Show
            mrawls Meredith Rawls added a comment - Yay! Please ensure the package has some docs and tests, and no extraneous directories or table file entries, as K-T and John point out. Looking forward to borrowing lots of what you've done for analysis_ap, so the more good life choices you can make here, the happier we will all be!
            Hide
            Parejkoj John Parejko added a comment -

            Meredith's comment suggests to me that we might want an `analysis_base` class, if the drp and ap codes are going to be sharing things (I don't know if they will, but it's worth considering).

            Show
            Parejkoj John Parejko added a comment - Meredith's comment suggests to me that we might want an `analysis_base` class, if the drp and ap codes are going to be sharing things (I don't know if they will, but it's worth considering).
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31027 ]
            Hide
            jbosch Jim Bosch added a comment -

            Meredith's comment suggests to me that we might want an `analysis_base` class, if the drp and ap codes are going to be sharing things

            Yup, this was at least vaguely part of the plan all along - we just decided to stand up something that worked before trying to figure out which bits we might want to factor out into a more general package.  I'd like to keep that separate from this RFC, but I think it might make sense to do either while standing up a corresponding AP package or just after (with either some duplication or a AP->DRP dependency as we work through the refactoring).

            I'd be interested to hear Sophie Reed 's thoughts on whether the boundary between DRP-specific and more general stuff has ended up pretty clear already in the current version, though.

            Show
            jbosch Jim Bosch added a comment - Meredith's comment suggests to me that we might want an `analysis_base` class, if the drp and ap codes are going to be sharing things Yup, this was at least vaguely part of the plan all along - we just decided to stand up something that worked before trying to figure out which bits we might want to factor out into a more general package.  I'd like to keep that separate from this RFC, but I think it might make sense to do either while standing up a corresponding AP package or just after (with either some duplication or a AP->DRP dependency as we work through the refactoring). I'd be interested to hear Sophie Reed 's thoughts on whether the boundary between DRP-specific and more general stuff has ended up pretty clear already in the current version, though.
            tjenness Tim Jenness made changes -
            Status Flagged [ 10606 ] Board Recommended [ 11405 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31067 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31136 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31361 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31427 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31535 ]
            Hide
            tjenness Tim Jenness added a comment -

            Sophie Reed is there a hold up on this RFC that is preventing it from being adopted?

            Show
            tjenness Tim Jenness added a comment - Sophie Reed is there a hold up on this RFC that is preventing it from being adopted?
            sophiereed Sophie Reed made changes -
            Status Board Recommended [ 11405 ] Adopted [ 10806 ]
            Hide
            sophiereed Sophie Reed added a comment -
            Show
            sophiereed Sophie Reed added a comment - Implementation ticket:  https://jira.lsstcorp.org/browse/DM-32711
            tjenness Tim Jenness made changes -
            Link This issue is triggering DM-32711 [ DM-32711 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 31650 ]
            sophiereed Sophie Reed made changes -
            Resolution Done [ 10000 ]
            Status Adopted [ 10806 ] Implemented [ 11105 ]
            tjenness Tim Jenness made changes -
            Remote Link This issue links to "Page (Confluence)" [ 32306 ]

              People

              Assignee:
              sophiereed Sophie Reed
              Reporter:
              sophiereed Sophie Reed
              Watchers:
              Colin Slater, Jim Bosch, John Parejko, Kian-Tat Lim, Lee Kelvin, Meredith Rawls, Sophie Reed, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins

                  No builds found.