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

Measure photometric repeatability and correctness of reported errors

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      1. Calculate and plot photometric variability across series of N images. Compare to reported photometric errors. Designed for N > 5.
      2. Calculate and plot Delta flux / sigma_flux for multiple observations of stars in field. This is related to 1. but is focused on N=2 to N=5.
      3. Fit uncertainty distribution vs. magnitude to identify any floor in the photometric uncertainty and to check performance vs. photon counts.

        Attachments

          Activity

          Hide
          swinbank John Swinbank added a comment -

          Per my comment on Github, I just want to reiterate that I hadn't actually finished – I'm intending to return and answer the fundamental question ("does this match what you'd like to see") when I'm not in a docs hackathon.

          A general comment is that I'm not sure how much detailed code review you're looking for here. There are quite a few issues that I'd regard as red flags in terms of merging to one of the core Science Pipelines packages, but I'm aware that this work is, at least in part, exploratory so the same level as scrutiny might not be appropriate. I guess it's up to you and Frossie Economou to decide how seriously to take my worries in the short term.

          Show
          swinbank John Swinbank added a comment - Per my comment on Github, I just want to reiterate that I hadn't actually finished – I'm intending to return and answer the fundamental question ("does this match what you'd like to see") when I'm not in a docs hackathon. A general comment is that I'm not sure how much detailed code review you're looking for here. There are quite a few issues that I'd regard as red flags in terms of merging to one of the core Science Pipelines packages, but I'm aware that this work is, at least in part, exploratory so the same level as scrutiny might not be appropriate. I guess it's up to you and Frossie Economou to decide how seriously to take my worries in the short term.
          Hide
          wmwood-vasey Michael Wood-Vasey added a comment -

          I find all of the comments helpful, but would like to reserve the right to punt design issues to future tickets when trying to close out current ones. But please do continue to make all comments, and feel free to highlight any particular issues that you think would be red flags.

          E.g., what of your current comments would you think of as red flags? (other than the good reminds along the lines of: "this function should have a docstring and a good name")

          The biggest red flag for me right now is that there are no actual tests of the logic and calculations in validate_drp itself. I think that should have been done along the way and will need a dedicated ticket to add on to the work done so far.

          This is certainly not yet a "core" Science Pipeline package.

          It's certainly meant as exploratory and will transition to something more formally supported (and more slowly evolving) as we fill out the capabilities and get a sense of how things shake down. It is my hope that with each improvement, added feature, and refactoring, the code will get better and clearer. I note that thanks to your prompting, the code is already better introspectively documented in Python than some of the core packages.

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - I find all of the comments helpful, but would like to reserve the right to punt design issues to future tickets when trying to close out current ones. But please do continue to make all comments, and feel free to highlight any particular issues that you think would be red flags. E.g., what of your current comments would you think of as red flags? (other than the good reminds along the lines of: "this function should have a docstring and a good name") The biggest red flag for me right now is that there are no actual tests of the logic and calculations in validate_drp itself. I think that should have been done along the way and will need a dedicated ticket to add on to the work done so far. This is certainly not yet a "core" Science Pipeline package. It's certainly meant as exploratory and will transition to something more formally supported (and more slowly evolving) as we fill out the capabilities and get a sense of how things shake down. It is my hope that with each improvement, added feature, and refactoring, the code will get better and clearer. I note that thanks to your prompting, the code is already better introspectively documented in Python than some of the core packages.
          Hide
          swinbank John Swinbank added a comment -

          Sorry for the delay getting this review completed. I've now finished looking through the code and have verified that I can run this and get sensible outputs (using DECam only, but I trust the same would work for CFHT). I have not done a detailed comparison of the algorithms used vs those we used for reporting S15 KPMs, but I believe they look plausible. I suggest David Nidever [X] checks to make sure he's happy with them.

          The main question you asked was:

          Do the plots and stdout put as shown in this ticket match what you'd like to see?

          And the answer there is basically yes. The output looks fine for an interested end user who wants to poke at some data, and maybe that's what you're aiming for at the moment. Ultimately, though, I guess this isn't how you want to present the outputs at all – presumably it all gets fed into a database backed, web-based QA system. For now, though, I have no objection to what's here.

          One question I would raise is why the validation routine isn't written as a command-line task. I fear the answer might be that they're just too complex to get to grips with, which I think is a fair criticism of the technology but perhaps not ultimately a good reason. Using the existing task framework would buy you command line parsing (including integration with the repository, etc, rather than hard-coding data IDs in defaultData functions), logging, standardized interfaces and so on. I think it should be a real concern to the project if they're dismissed for work outside Science Pipelines.

          The code has been much improved by the changes you made since my previous partial review. I've made a number of additional comments on the code on GitHub and would appreciate it if you take a look at them. Very few of them are substantive changes though. (One minor nit I didn't mention but which occurs quite frequently is trailing whitespace. Not a big deal, but drives me nuts!)

          I absolutely agree with your comment about the lack of tests: this is probably the single biggest reason why we wouldn't consider merging something like this into Science Pipelines. In general, in pipeline-land we wouldn't mergie to master until we feel a particular feature is "done": it's not just functional, but it's documented, the code is reasonably polished, the test suite is running, and so on. That's what makes me a little leery about giving this a green light. However, I do understand that you're operating in a different regime where the priority is to prototype rather than to roll out production quality code. As long as we're clear that this is just a prototype rather than a finished article, I'll leave it up to you Frossie Economou as to whether this is something SQuaRE wants to support.

          With that caveat, and assuming you take another look at the PR (and, in particular, my comment on magNormDiff), this is good to go.

          Show
          swinbank John Swinbank added a comment - Sorry for the delay getting this review completed. I've now finished looking through the code and have verified that I can run this and get sensible outputs (using DECam only, but I trust the same would work for CFHT). I have not done a detailed comparison of the algorithms used vs those we used for reporting S15 KPMs, but I believe they look plausible. I suggest David Nidever [X] checks to make sure he's happy with them. The main question you asked was: Do the plots and stdout put as shown in this ticket match what you'd like to see? And the answer there is basically yes. The output looks fine for an interested end user who wants to poke at some data, and maybe that's what you're aiming for at the moment. Ultimately, though, I guess this isn't how you want to present the outputs at all – presumably it all gets fed into a database backed, web-based QA system. For now, though, I have no objection to what's here. One question I would raise is why the validation routine isn't written as a command-line task. I fear the answer might be that they're just too complex to get to grips with, which I think is a fair criticism of the technology but perhaps not ultimately a good reason. Using the existing task framework would buy you command line parsing (including integration with the repository, etc, rather than hard-coding data IDs in defaultData functions), logging, standardized interfaces and so on. I think it should be a real concern to the project if they're dismissed for work outside Science Pipelines. The code has been much improved by the changes you made since my previous partial review. I've made a number of additional comments on the code on GitHub and would appreciate it if you take a look at them. Very few of them are substantive changes though. (One minor nit I didn't mention but which occurs quite frequently is trailing whitespace. Not a big deal, but drives me nuts!) I absolutely agree with your comment about the lack of tests: this is probably the single biggest reason why we wouldn't consider merging something like this into Science Pipelines. In general, in pipeline-land we wouldn't mergie to master until we feel a particular feature is "done": it's not just functional, but it's documented, the code is reasonably polished, the test suite is running, and so on. That's what makes me a little leery about giving this a green light. However, I do understand that you're operating in a different regime where the priority is to prototype rather than to roll out production quality code. As long as we're clear that this is just a prototype rather than a finished article, I'll leave it up to you Frossie Economou as to whether this is something SQuaRE wants to support. With that caveat, and assuming you take another look at the PR (and, in particular, my comment on magNormDiff ), this is good to go.
          Hide
          wmwood-vasey Michael Wood-Vasey added a comment - - edited

          Thanks for the comments.

          I summarize the discussion on the GitHub ticket and the key points (and additional tickets) above as:

          1. Add tests of key calculations and ancillary utilities. DM-5098
          2. Merge validateCfht and validateDecam. Put the differences in data configuration files, not the code. DM-4901.
          3. Use TransformTask for the calibrated photometry. Unfortunately this is not supremely well documented. I need to learn how to set this up. Now DM-5097
          4. Make this a Task. Agreed. Part of DM-2050. But just making this a Task shouldn't interfere with the overall test harness design, so I'll make this a Task as DM-5096.

          I did a lot of whitespace and unused import/function cleanup for DM-4956 . Sorry to have not done it here. In the interest of pragmatism I'll merge this DM-4848 to master and make sure DM-4956, which I will submit for review in a few hours, is really clean on this.

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - - edited Thanks for the comments. I summarize the discussion on the GitHub ticket and the key points (and additional tickets) above as: 1. Add tests of key calculations and ancillary utilities. DM-5098 2. Merge validateCfht and validateDecam. Put the differences in data configuration files, not the code. DM-4901 . 3. Use TransformTask for the calibrated photometry. Unfortunately this is not supremely well documented. I need to learn how to set this up. Now DM-5097 4. Make this a Task. Agreed. Part of DM-2050 . But just making this a Task shouldn't interfere with the overall test harness design, so I'll make this a Task as DM-5096 . I did a lot of whitespace and unused import/function cleanup for DM-4956 . Sorry to have not done it here. In the interest of pragmatism I'll merge this DM-4848 to master and make sure DM-4956 , which I will submit for review in a few hours, is really clean on this.
          Hide
          wmwood-vasey Michael Wood-Vasey added a comment -

          Merged to master.

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - Merged to master.

            People

            • Assignee:
              wmwood-vasey Michael Wood-Vasey
              Reporter:
              wmwood-vasey Michael Wood-Vasey
              Reviewers:
              John Swinbank
              Watchers:
              Hsin-Fang Chiang, John Swinbank, Michael Wood-Vasey
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel