Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ip_isr
-
Labels:None
-
Story Points:1
-
Epic Link:
-
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
Where does this stand? The code has merged, but the ticket is still "In Review".
Paul Price
added a comment - Where does this stand? The code has merged, but the ticket is still "In Review". 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.
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?
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 ? 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!
Done.