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

Migrate all metrics from ap.verify.measurements

    XMLWordPrintable

Details

    • Improvement
    • Status: Done
    • Resolution: Done
    • None
    • ap_verify
    • None
    • 6
    • AP S19-2, AP S19-3, AP S19-4
    • Alert Production

    Description

      As a proof of concept, DM-16017 only implemented a limited set of metrics. Once the MetricTask framework is mature, we should reimplement all existing metrics as MetricTasks and retire the ap.verify.measurements module.

      Attachments

        Issue Links

          Activity

            krzys Krzysztof Findeisen added a comment - - edited

            As noted in the AP pipeline meeting, the timing metric is a bit vague about what it's actually timing (in particular, the documentation for TimingMetricTask.run says wall-clock time, but the documentation for TimingMetricTask itself or individual metrics does not). Since this ticket must deal with metric presentation issues anyway, modify the descriptions to be explicit. Likewise, check that other metrics' descriptions are as precise and unambiguous as possible.

            krzys Krzysztof Findeisen added a comment - - edited As noted in the AP pipeline meeting , the timing metric is a bit vague about what it's actually timing (in particular, the documentation for TimingMetricTask.run says wall-clock time, but the documentation for TimingMetricTask itself or individual metrics does not). Since this ticket must deal with metric presentation issues anyway, modify the descriptions to be explicit. Likewise, check that other metrics' descriptions are as precise and unambiguous as possible.

            Hi ebellm and jsick, would you be willing to review this ticket? Unfortunately, the work got a bit out of hand, so the changes are large and a bit tangled up, particularly in ap_verify. I apologize for that.

            jsick, please review the changes to verify and ap_verify, which are partly improvements to MetricTask infrastructure and partly fixes and workarounds I couldn't decouple from them.

            ebellm, please review the changes to verify_metrics, ip_diffim, and ap_association, which deal more with metric/measurement definitions.

            krzys Krzysztof Findeisen added a comment - Hi ebellm and jsick , would you be willing to review this ticket? Unfortunately, the work got a bit out of hand, so the changes are large and a bit tangled up, particularly in ap_verify . I apologize for that. jsick , please review the changes to verify and ap_verify , which are partly improvements to MetricTask infrastructure and partly fixes and workarounds I couldn't decouple from them. ebellm , please review the changes to verify_metrics , ip_diffim , and ap_association , which deal more with metric/measurement definitions.
            ebellm Eric Bellm added a comment -

            Hi krzys--see comments on github.

            My only large comment is that I'm worried now about the name "science sources" (which you probably got from me!).  It doesn't really match the rest of LSST nomenclature (are DIASources not used for science?).  Should we consider "direct imaging sources" or "calexp sources" or "PVI sources"?  Ideas welcome.

            ebellm Eric Bellm added a comment - Hi krzys --see comments on github. My only large comment is that I'm worried now about the name "science sources" (which you probably got from me!).  It doesn't really match the rest of LSST nomenclature (are DIASources not used for science?).  Should we consider "direct imaging sources" or "calexp sources" or "PVI sources"?  Ideas welcome.

            ebellm, all names (including "science sources") were taken directly from the previous versions of the metrics. I don't mind changing the name, but I think if we do then we should change it everywhere (notebooks, Confluence, etc.). Maybe we should discuss the name in #dm-alert-prod or group meeting?

            krzys Krzysztof Findeisen added a comment - ebellm , all names (including "science sources") were taken directly from the previous versions of the metrics. I don't mind changing the name, but I think if we do then we should change it everywhere (notebooks, Confluence, etc.). Maybe we should discuss the name in #dm-alert-prod or group meeting?
            ebellm Eric Bellm added a comment -

            Sounds good--I'll add it to the Monday meeting agenda.

            On Fri, Mar 8, 2019 at 3:35 PM Krzysztof Findeisen (JIRA) <jira-dm@lsst.org>

            ebellm Eric Bellm added a comment - Sounds good--I'll add it to the Monday meeting agenda. On Fri, Mar 8, 2019 at 3:35 PM Krzysztof Findeisen (JIRA) <jira-dm@lsst.org>

            This looks good from my perspective. Let me know if there's something I should have been looking at in more detail.

            jsick Jonathan Sick added a comment - This looks good from my perspective. Let me know if there's something I should have been looking at in more detail.
            jsick Jonathan Sick added a comment - - edited

            Ok, I'll pick up threads here from both the verify and ap_verify PR discussions. I have a better appreciation of where this is going now, and the potential issues. To be completely honest and transparent, I haven't worked on lsst.verify or QA in general for about 1.5 years, so I don't have much context with what's going on and hence why I've been generally unopinonated.

            I see now specifically that these task bases are going into the root of lsst.verify. As I said in https://jira.lsstcorp.org/browse/DM-17543?focusedCommentId=185663&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-185663 it's up to you, but it might be a good idea to collect tasks task baseclasses in a lsst.verify.tasks subpackage. As I said before, this doesn't mean creating a new EUPS package (which I don't want), but it does organize the APIs). This specifically addresses your concern in https://github.com/lsst/ap_verify/pull/63#issuecomment-473024245 that adding TimingMetricTask to lsst.verify might pollute that namespace.

            (But again, my level of thinking here is probably pretty shallow. You're the lsst.verify expert right now!)

            jsick Jonathan Sick added a comment - - edited Ok, I'll pick up threads here from both the verify and ap_verify PR discussions. I have a better appreciation of where this is going now, and the potential issues. To be completely honest and transparent, I haven't worked on lsst.verify or QA in general for about 1.5 years, so I don't have much context with what's going on and hence why I've been generally unopinonated. I see now specifically that these task bases are going into the root of lsst.verify . As I said in https://jira.lsstcorp.org/browse/DM-17543?focusedCommentId=185663&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-185663  it's up to you, but it might be a good idea to collect tasks task baseclasses in a lsst.verify.tasks subpackage. As I said before, this doesn't mean creating a new EUPS package (which I don't want), but it does organize the APIs). This specifically addresses your concern in https://github.com/lsst/ap_verify/pull/63#issuecomment-473024245  that adding TimingMetricTask to lsst.verify might pollute that namespace. (But again, my level of thinking here is probably pretty shallow. You're the lsst.verify expert right now!)

            Thanks for the feedback!

            krzys Krzysztof Findeisen added a comment - Thanks for the feedback!

            People

              krzys Krzysztof Findeisen
              krzys Krzysztof Findeisen
              Eric Bellm, Jonathan Sick
              Eric Bellm, Jonathan Sick, Krzysztof Findeisen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.