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

Construct master calibs for obs_ctio0m9

    XMLWordPrintable

    Details

    • Story Points:
      12
    • Epic Link:
    • Sprint:
      DRP S17-5, DRP S17-6, DRP F17-1
    • Team:
      Data Release Production

      Description

      Get the pipe_drivers contruct_*.py scripts running to make master calibs for obs_ctio0m9.

      Currently this doesn't work, and a number of bug fixes are/will be necessary, likely in both pipe_drivers and the prototype obs_ctio0m9.

        Attachments

          Issue Links

            Activity

            Hide
            krughoff Simon Krughoff added a comment -

            I'm pretty uncomfortable with the changes to pipe_tasks. Passing around mappers and accessing the internal standardization methods seems dangerous. I understand the desire not to duplicate code, so maybe that's why you didn't use a translate function. That can be avoided by putting the code in a free function and calling it in both translate_taiObs and std_raw_md. That would mean you do not need the changes in pipe_tasks.

            Another general comment is that the coding standard asks for docstrings for every method and class. There are many methods in this changes et that have no docstring at all and many others where the docstring does not adhere to our standard (i.e. naming all input and output variables).

            I'm not quite done with the review, but I'll post this now so you can look at them.

            Show
            krughoff Simon Krughoff added a comment - I'm pretty uncomfortable with the changes to pipe_tasks . Passing around mappers and accessing the internal standardization methods seems dangerous. I understand the desire not to duplicate code, so maybe that's why you didn't use a translate function. That can be avoided by putting the code in a free function and calling it in both translate_taiObs and std_raw_md . That would mean you do not need the changes in pipe_tasks . Another general comment is that the coding standard asks for docstrings for every method and class. There are many methods in this changes et that have no docstring at all and many others where the docstring does not adhere to our standard (i.e. naming all input and output variables). I'm not quite done with the review, but I'll post this now so you can look at them.
            Hide
            krughoff Simon Krughoff added a comment -

            I've completed my review. I'd like to come up with a solution that does not include the pipe_tasks changes you have on this ticket. I would also like to make sure docstrings are up to standard. The rest of my suggestions are relatively minor.

            Show
            krughoff Simon Krughoff added a comment - I've completed my review. I'd like to come up with a solution that does not include the pipe_tasks changes you have on this ticket. I would also like to make sure docstrings are up to standard. The rest of my suggestions are relatively minor.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Simon Krughoff I believe I have dealt with all your comments now. Could you take a look and confirm please?

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Simon Krughoff I believe I have dealt with all your comments now. Could you take a look and confirm please?
            Hide
            krughoff Simon Krughoff added a comment - - edited

            Looks great. The only minor thing is that I don't see how you've dealt with the fact that the same code exists in the notebook as exists in the pre-burner script. It's not a big deal to me, but thought I'd mention it. Feel free to merge.

            Edit: I was confused. The duplication was resolved. I was looking at another script that is not a duplication of the notebook.

            Show
            krughoff Simon Krughoff added a comment - - edited Looks great. The only minor thing is that I don't see how you've dealt with the fact that the same code exists in the notebook as exists in the pre-burner script. It's not a big deal to me, but thought I'd mention it. Feel free to merge. Edit: I was confused. The duplication was resolved. I was looking at another script that is not a duplication of the notebook.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Merged.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Merged.

              People

              Assignee:
              mfisherlevine Merlin Fisher-Levine
              Reporter:
              mfisherlevine Merlin Fisher-Levine
              Reviewers:
              Simon Krughoff
              Watchers:
              John Swinbank, Merlin Fisher-Levine, Robert Lupton, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.