DM-15862: Reduce ISR code duplication between ip_isr, obs_subaru, and obs_decam #167
Conversation
There was a problem hiding this comment.
OK I got tired of doing it everywhere, but commented out code should really not make it to master. If it's commented, it's not needed. A plain english comment could be left if something really needs to be explained, but leaving commented code in is more confusing than anything else. Just take a pass through this diff and remove commented code where ever it exists.
Other than that, things look pretty good.
| # Nothing to mask! | ||
| return | ||
|
|
||
| if False: |
There was a problem hiding this comment.
if False statements are generally frowned upon in code on master as it's essentially a block of commented out code. I suppose this could be ok if it is associated with ticketed work that will remove the if False, but if not, it really should be removed. You can leave a comment with a SHA1 for where the code was removed so someone could look it up, if you want.
There was a problem hiding this comment.
I've added ticket DM-16806 to address this issue as well. I'm hesitant to change inherited code that I don't have readily available test data for.
| @@ -120,6 +115,7 @@ def run(self, sensorRef, exposure): | |||
|
|
|||
| model *= exposure.getInfo().getVisitInfo().getExposureTime() | |||
| exposure.image.array -= model | |||
| # CZW: Should these methods be moved into the class? They're not useful outside it, right? | |||
There was a problem hiding this comment.
Did this get resolved? If so, what's the answer?
There was a problem hiding this comment.
I've removed the comment and left the methods independent of the class. This is probably overly conservative, but working through the straylight correction is a bit out of the scope of the ticket. I've added a new ticket DM-16805 to handle a future examination and rewrite of the subaru straylight code.
czwa
force-pushed
the
tickets/DM-15862
branch
4 times, most recently
from
59945dd
to
30ddabf
Compare
The ISR code was split between ip_isr and the obs_ packages, with the ip_isr and obs_subaru containing parallel ISR reduction pathways. This ticket merges those pathways into a single set of ISR functions in the ip_isr package, removing the duplication of code. The merged codebase also makes additional useful functions from the obs_subaru ISR available to other cameras, and the new ISR code now profiles debug points, thumbnail images, and post-correction statistics to check processing quality. The Subaru specific ISR task classes have been removed in favor of the one in ip_isr. The majority of the methods have been migrated to ip_isr, reducing the impact of the change on Subaru processing. Ticket pointers for still unresolved issues/code style issues are included in comments. The ISR metadata is now included in the HscMapper to allow this to be persisted.
czwa
force-pushed
the
tickets/DM-15862
branch
from
30ddabf
to
a9ca040
Compare
The Subaru specific ISR task classes have been removed in favor of the
one in ip_isr. The majority of the methods have been migrated to
ip_isr, reducing the impact of the change on Subaru processing.
The ISR metadata is now included in the HscMapper to allow this to be
persisted.