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

crosstalk correction was moved above assembleCcd, which broke it

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_isr
    • Labels:
      None
    • Story Points:
      1
    • Sprint:
      DRP F18-2
    • Team:
      Alert Production

      Description

      In cc1e91fc90, running the crosstalk task was moved above assembleCcd, which broke the crosstalk correction in ip_isr.  Due to the fact that HSC uses a custom ISR this fact was not visible in ci_hsc, and in fact no tests were checking for this error.  The tests of the crosstalk task itself were unaffected by changes in isrTask.  The problem only showed up when we started processing lsstCamera data (ts8 and in particular phosim) through obs_lsstCam.

      I propose that we revert this change.  It was made to support DECam, but I do not understand why it was necessary so I think it's better to fix obs_decam than ip_isr, but would like to understand why the DECam code was written this way before taking a final decision.  In discussion, Meredith pointed out that she didn't want to have to assemble CCDs to process inter-CCD crosstalk, but I don't think that this would be necessary (you'd use the Detector to iterate over the amplifier segments, applying suitable flips as described in the Detector's amplifier objects)

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Done.

            Show
            swinbank John Swinbank added a comment - Done.
            Hide
            price Paul Price added a comment -

            Where does this stand? The code has merged, but the ticket is still "In Review".

            Show
            price Paul Price added a comment - Where does this stand? The code has merged, but the ticket is still "In Review".
            Hide
            swinbank John Swinbank added a comment -

            Paul Price — If you could take a quick look and check that you don't object to what got merged that would be great. Or, of course, if you do object, let us know and Meredith Rawls and I will make any necessary fixes.

            Show
            swinbank John Swinbank added a comment - Paul Price — If you could take a quick look and check that you don't object to what got merged that would be great. Or, of course, if you do object, let us know and Meredith Rawls and I will make any necessary fixes.
            Hide
            price Paul Price added a comment -

            The only comment I have is that both the commit messages consist only of a single summary line. It would have been better to include more detail, in particular why the change was necessary and why the particular solution was adopted. Someone shouldn't need to refer to Jira or Slack to discover these when they visit the code next year to fix something. This is not something that's easy to fix because the commits are already on master. But in the future, please could we follow the excellent recommendations in the section on commit messages in the developer guide?

            Show
            price Paul Price added a comment - The only comment I have is that both the commit messages consist only of a single summary line. It would have been better to include more detail, in particular why the change was necessary and why the particular solution was adopted. Someone shouldn't need to refer to Jira or Slack to discover these when they visit the code next year to fix something. This is not something that's easy to fix because the commits are already on master. But in the future, please could we follow the excellent recommendations in the section on commit messages in the developer guide ?
            Hide
            mrawls Meredith Rawls added a comment -

            Thanks Paul Price, that's a good point. Following the recommendations would have helped answer the original question about why the ISR order changed more quickly, too. I will remember to include more comprehensive commit messages next time!

            Show
            mrawls Meredith Rawls added a comment - Thanks Paul Price , that's a good point. Following the recommendations would have helped answer the original question about why the ISR order changed more quickly, too. I will remember to include more comprehensive commit messages next time!

              People

              Assignee:
              mrawls Meredith Rawls
              Reporter:
              rhl Robert Lupton
              Reviewers:
              Paul Price
              Watchers:
              Eric Bellm, James Chiang, John Swinbank, Meredith Rawls, Paul Price, Robert Lupton, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.