Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ip_isr
-
Labels:
-
Story Points:10
-
Epic Link:
-
Sprint:Science Pipelines DM-W15-4, Science Pipelines DM-S15-1, Science Pipelines DM-S15-2
-
Team:Alert Production
Description
Once the API is designed and signed off on, the API will need to be implemented and tested. This will require updating all obs_* packages that use the current interface.
Attachments
Issue Links
Activity
This looks fine to me. As soon as Russell Owen reviews this, I'm happy for it to be merged.
Here is my review of ip_isr (I will post my review of pipe_tasks separately).
Overall this looks like a nice improvement. I do have small suggestions...
isrTask.py:
- I'm not keen on the term "frame". I think you mean "image" or "data", and I'd prefer to reserve "frame" for other purposes. Thus I suggesting renaming method "getIsrFrames" to some choice from "get[Calibration|Isr][Data|Images]", such as "getCalibrationData". Then rename the config parameter "doAssembleIsrFrames" accordingly.
- doAssembleIsrFrames documentation says "detrend/calibration/isr frames" but we don't use the term "detrend". I think "Assemble calibration images?" would suffice. Does it really need to be a different flag than "doAssembleCcd" (do you have a use case)? I assume yes, and if so, I sure hope "doAssembleIsrFrames" is sufficiently fine-grained; it would be awful if some types of calibration data needed assembly and some didn't. And yes at that point a subclass would be called for.
- For the "apply" method please document what happens if an input is None. I assume that step is skipped.
- I suggest renaming "applyToSensorRef" to "applySensorRef" or "applyDataRef" for cripsness.
- What is this test for (please document the need in the code). Do we really need it?
if ccdExposure.getBBox().contains(amp.getBBox()):
fringe.py:
Overall this is a fine change. I have a few picky suggestions:
- A pre-existing problem is that the subtract method does not check that len(fringes) == len(solution). Would you mind adding such a test and raising an exception if not (I suggest raising RuntimeError or something similar, not TaskError, since it indicates a bug).
- Another pre-existing problem is lack of documentation for FringeTask. I suggest you file a ticket (unless you want to add the information; it's a lot).
- I suggest moving readFringes just before removeFringes (like readIsrData is to apply in IsrTask), since those are the two main methods and this lists them in the order in which they will be called.
- Consider renaming removeFringes->apply and removeFringesWithDataRef->applyDataRef to be consistent with your IsrTask naming scheme.
Thank you for the review. If u/yusra/DM-1299 looks OK (knowing that the bulk of the example and unit test updates will be in DM-1151), I'll consolidate the commits in a ticket branch and merge.
The hold-up was that I started working on DM-1151, the scope of which continued to expand to a greater variety of input images. The work on DM-1151 actually resulted in a few changes to the API and a number of updates on this branch.
- Fringe.py: While removing the dataRefs from testFringe.py, it became clear that the interface for fringeTask.run() was still too specific. It should take a fringe exposure or list of fringe exposures and compute the fringe amplitudes itself (instead accepting a struct with positions and amplitudes already calculated.)
- I had second thoughts on naming the primary method of reuse "apply" instead of "run." Whereas IsrTask is technically not an commandlineTask as the moment, it has commandlineTask potential. Of all the virtues, predictability won out. I changed the method names to run and runDataRef.
- Testing on arbitrary images exposed some brittleness, which resulted in some one-off additions and input checks.
Response to comments:
isrTask:
- "Frames" changed to "Exposures" throughout. I opted for "Isr" over "Calibration" because I think of photometric/astrometric calibration and Calib when I hear "calibration".
- Added a comment for the condition that checks whether the image actually is covered by the amp before operating on it.
- By default doAssembleCcd = True and doAssembleIsrExposures = False The only appropriate scenario for doAssembleCcd = False, is when using the run method on a single amp. Indeed, the reason I added it was for the one amp example in obs_lsstSim. We may be able to remove this when we make assembleCcd more robust to edge cases.
Fringe:
- Made all changed you suggested: added the runtimeError for non-matching scale array and fringe array lengths. Moved the methods around, and simplified method names.
- A ticket on fringe task is open as
DM-917: "Provide Task documentation for FringeTask"
Edits are on u/yusra/
DM-1299Simon, can you review the obs_lsstSim changes?
Russell, can you review the ip_isr and pipe_tasks changes?
Some notes:
ip_isr/pipe_tasks - Summary of method name discussion: IsrTask has a clearly defined primary method which removes the instrument signature. For this reason, a name such as "removeInstrumentSignature()" or "doISR" is unnecessary since IsrTask is unlikely to be used for its other methods. A simple name such as "run" is appropriate, but "run" should be reserved for commandline tasks. For these reasons, I settled on "apply."
obs_lsstSim - Functionality is untested on a full chip. This should be tested when a full chip of simulated lsst data becomes available. The new unit test operates on one amp in tests/data and does not attempt to assembleCcd.
Creating a unit test on an amp-sized image uncovered unusual behavior in assembleCcd. If amp-sized exposure is passed in as a ccdExposure, assembleCcd duplicates it across the chip. I expected assembleCcd to either:
This brought up the question of what to do when an amplifier table is missing data. I think this requires some thought, and did not address it in this ticket.