# Add mask plane to indicate no brighter fatter correction around edges

XMLWordPrintable

#### Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
6
• Team:
Data Release Production

#### Description

The brighter fatter correction convolves the image with a kernel to do the correction. In the border region of the image where the convolution is not valid there is no correction applied. We need to add a bit in the mask plane to indicate this.

#### Activity

No builds found.
Bob Armstrong created issue -
Field Original Value New Value
Issue Type Story [ 10001 ] Improvement [ 4 ]
 Component/s ip_isr [ 10730 ]
Hide
John Swinbank added a comment -

Assigning to AP since they lead on ip_isr. Simon Krughoff, do feel free to object!

Show
John Swinbank added a comment - Assigning to AP since they lead on ip_isr . Simon Krughoff , do feel free to object!
 Team Alert Production [ 10300 ]
Gabriele Comoretto [X] (Inactive) made changes -
 Team Alert Production [ 10300 ] Data Release Production [ 10301 ]
Hide
John Swinbank added a comment -

No longer an AP problem — Christopher Waters & Merlin Fisher-Levine are the gurus here now. As far as I can see from a cursory glance, we're still not setting mask bits in the areas which are not being corrected. Merlin & Chris, do you concur? Seems like it would be an easy fix...

Show
John Swinbank added a comment - No longer an AP problem — Christopher Waters & Merlin Fisher-Levine are the gurus here now. As far as I can see from a cursory glance, we're still not setting mask bits in the areas which are not being corrected. Merlin & Chris, do you concur? Seems like it would be an easy fix...
Hide
Merlin Fisher-Levine added a comment -

I think it would. A few questions though: 1) Do we need a new mask plane for this? 2) If they're not easily spared, would just using EDGE be sufficient? I imagine the sizes would be similar, and because I'm not quite sure of the use-case for this new plane, I'm not sure how precise it needs to be. It's also possible that EDGE turns up in places that aren't the actual edge of the sensor, meaning this wouldn't be appropriate at all, so I'm not sure.

But yes, if we can spare making a new maskplane for this then it should be easy.

Show
Merlin Fisher-Levine added a comment - I think it would. A few questions though: 1) Do we need a new mask plane for this? 2) If they're not easily spared, would just using EDGE be sufficient? I imagine the sizes would be similar, and because I'm not quite sure of the use-case for this new plane, I'm not sure how precise it needs to be. It's also possible that EDGE turns up in places that aren't the actual edge of the sensor, meaning this wouldn't be appropriate at all, so I'm not sure. But yes, if we can spare making a new maskplane for this then it should be easy.
Hide
Christopher Waters added a comment -

I'm happy to think about this a bit more, but my initial impression is that we should be setting IsrTask.config.numEdgeSuspect to the half-kernel size of the brighter-fatter kernel.  Making this automatic (if a brighter fatter kernel is supplied, and is to be applied, then numEdgeSuspect should be at least 0.5 * kernel size) feels like the right long term solution.

I have no objection to adding additional mask planes to define this, but worry about maintaining the propagation of those planes to downstream code (i.e. with HSC's GUIDER plane being ignored).  Is there a list somewhere defining plane names and how analysis code should be treating them?

Show
Christopher Waters added a comment - I'm happy to think about this a bit more, but my initial impression is that we should be setting IsrTask.config.numEdgeSuspect to the half-kernel size of the brighter-fatter kernel.  Making this automatic (if a brighter fatter kernel is supplied, and is to be applied, then numEdgeSuspect should be at least 0.5 * kernel size) feels like the right long term solution. I have no objection to adding additional mask planes to define this, but worry about maintaining the propagation of those planes to downstream code (i.e. with HSC's GUIDER plane being ignored).  Is there a list somewhere defining plane names and how analysis code should be treating them?
 Status To Do [ 10001 ] In Progress [ 3 ]
 Assignee Christopher Waters [ cwaters ]
Hide
John Swinbank added a comment -

Christopher Waters — noting you set this to “in progress” — I don't think this is urgent, but if you are free to work on it, then obviously that's great. Unfortunately, I don't know of any list of mask planes beyond what exists in the code; see e.g. DM-2297.

Show
John Swinbank added a comment - Thanks both for your comments. Christopher Waters — noting you set this to “in progress” — I don't think this is urgent, but if you are free to work on it, then obviously that's great. Unfortunately, I don't know of any list of mask planes beyond what exists in the code; see e.g. DM-2297 .
Hide
Christopher Waters added a comment -

It was a quick enough thing to do, if SUSPECT is an acceptable mask plane (plus it was a nice break from other tickets).

Do either of you have time to do the quick review?

Show
Christopher Waters added a comment - It was a quick enough thing to do, if SUSPECT is an acceptable mask plane (plus it was a nice break from other tickets). Do either of you have time to do the quick review?
 Reviewers John Swinbank [ swinbank ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
John Swinbank added a comment -

Thanks for the quick fix!

Some minor code comments on the PR. Beyond that —

• I claim no expertise on the semantics of mask planes, but I wonder if BAD might be better than SUSPECT? I naïvely assume we would simply want to ignore data without BF correction applied, but I would defer to Jim Bosch or Bob Armstrong as to whether I'm right.
• Any chance of adding a test case?
Show
John Swinbank added a comment - Thanks for the quick fix! Some minor code comments on the PR. Beyond that — I claim no expertise on the semantics of mask planes, but I wonder if BAD might be better than SUSPECT? I naïvely assume we would simply want to ignore data without BF correction applied, but I would defer to Jim Bosch or Bob Armstrong as to whether I'm right. Any chance of adding a test case?
Hide
Jim Bosch added a comment -

I don't think SUSPECT is quite right (though perhaps it should be repurposed so it could be - right now it really means "high enough that nonlinearity correction may be inaccurate, but not saturated", and we've long thought we should just fix the nonlinearity corrections instead of having it).  But I don't necessarily have a better suggestion; BAD means we'll just get rid of those areas entirely (it's what we use for defects).  Maybe EDGE, which is what we use for other cases where kernel width means we lose pixels on the outside?

I think the important question is what downstream processing will do with whatever mask bit you choose.  My familiarity with those steps w.r.t. mask planes is not what it once was, so I'm afraid I'm not much help there.  Yusra AlSayyad or Lauren MacArthur may be in a better position to guess.

Show
Jim Bosch added a comment - I don't think SUSPECT is quite right (though perhaps it should be repurposed so it could be - right now it really means "high enough that nonlinearity correction may be inaccurate, but not saturated", and we've long thought we should just fix the nonlinearity corrections instead of having it).  But I don't necessarily have a better suggestion; BAD means we'll just get rid of those areas entirely (it's what we use for defects).  Maybe EDGE, which is what we use for other cases where kernel width means we lose pixels on the outside? I think the important question is what downstream processing will do with whatever mask bit you choose.  My familiarity with those steps w.r.t. mask planes is not what it once was, so I'm afraid I'm not much help there.  Yusra AlSayyad or Lauren MacArthur may be in a better position to guess.
Hide
John Swinbank added a comment -

Changes on the PR look fine to me — thanks!

Show
John Swinbank added a comment - Changes on the PR look fine to me — thanks!
 Status In Review [ 10004 ] Reviewed [ 10101 ]
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Epic Link DM-20345 [ 337970 ]
 Story Points 3
 Story Points 3 6
 Epic Link DM-20345 [ 337970 ] DM-20164 [ 323028 ]

#### People

Assignee:
Christopher Waters
Reporter:
Bob Armstrong
Reviewers:
John Swinbank
Watchers:
Bob Armstrong, Christopher Waters, Jim Bosch, John Swinbank, Merlin Fisher-Levine