ccdmask (rebased and without master) #432
Conversation
…or use the percentile_filter in scipy which is a slightly different algorithm.
use try/except to validate type and shape of ratio
|
@astropy/ccdproc-maintainers I made some changes (mostly PEP stuff). Would be very happy if you could review this. Additionally I was thinking about putting the badcolumn part in a seperate function because it's mostly independant from the median/percentile part and it could also be useful in combination with other masking functions (for example astroscrappy or if one already has a "valid" mask). |
|
Will do a quick review later today. |
|
sphinx failure is unrelated: astropy/astropy#5568 |
|
Sorry @mwcraig I was just testing the new "review request" feature here. I hope you don't mind. |
There was a problem hiding this comment.
The changes requested are minor. @MSeifert04 -- I can do these later today if you do not have time, just let me know.
One broader question, the answer to which is, I think, no: Should the code try to enforce the recommendations for function parameters mentioned in the documentation? Am leaning towards no since the default parameter values are sensible and consistent with those guidelines.
| Gaps of undetected pixels along the column direction of length less | ||
| than this amount are also flagged as bad pixels. | ||
|
|
||
| Notes |
There was a problem hiding this comment.
Very minor quibble: the Parameters section should come before Notes
| """ | ||
| Uses method based on the IRAF ccdmask task to generate a mask based on the | ||
| given input. | ||
|
|
There was a problem hiding this comment.
Please add a statement explaining which index this function considers line and which is column. Looks like line is the first index and column is the second.
|
|
||
| mask = ~np.isfinite(ratio.data) | ||
| medsub = (ratio.data - | ||
| ndimage.median_filter(ratio.data, size=(nlmed, ncmed))) |
There was a problem hiding this comment.
Isn't there a median filter in ccdproc now? We should use that...
There was a problem hiding this comment.
Yes there is. I haven't changed it because that's just a wrapper around ndimage.median_filter and at this line we're already working on the pure numpy arrays so there is no difference.
If you like it should be as easy as replacing the ndimage.median_filter with median_filter.
| csum_sigma = np.ma.MaskedArray(np.sqrt(c2 - c1 - csum)) | ||
| colmask = _sigma_mask(csum.filled(1), csum_sigma, | ||
| lsigma, hsigma) | ||
| block_mask[:, :] |= colmask[None, :] |
There was a problem hiding this comment.
Though I know it is literally the same thing, I find np.newaxis easier to understand when reading the code than None
| for line in six.moves.range(0, nlines - ngood - 1): | ||
| if mask[line, col]: | ||
| for i in six.moves.range(2, ngood + 2): | ||
| lend = line+i |
There was a problem hiding this comment.
PEP8: Please add spaces around the operator.
|
|
||
| ngood: `int`, optional | ||
| Gaps of undetected pixels along the column direction of length less | ||
| than this amount are also flagged as bad pixels. |
There was a problem hiding this comment.
pixels. → pixels, if they are between pixels masked in that column.
| from ..ccddata import CCDData | ||
|
|
||
|
|
||
| def test_ccdmask_pixels(): |
There was a problem hiding this comment.
Could a comment be added here about the intent of the test arrays? Are they snippets from larger images, should they have bad pixels or are those only added in the body of the test, etc.
Doesn't need to be super long, just enough so it is clearer what can (and cannot) be modified in the future.
@joshwalawender can maybe address this most easily.
There was a problem hiding this comment.
Sorry the additional tests incorrectly hide this comment. Could you repost it?
No worries! I need to check whether notifications can be set up for this. Could be super-useful for assigning work. |
|
|
||
|
|
||
| def test_ccdmask_pixels(): | ||
| flat1 = CCDData(np.array([[ |
There was a problem hiding this comment.
[repost]
Could a comment be added here about the intent of the test arrays? Are they snippets from larger images, should they have bad pixels or are those only added in the body of the test, etc.
Doesn't need to be super long, just enough so it is clearer what can (and cannot) be modified in the future.
@joshwalawender can maybe address this most easily.
|
Merging, thanks @MSeifert04 and @joshwalawender |
Supersedes #414.
I manually applied the changes made after the master merge. I discarded more than 10 branches because I couldn't get cherry-picking working. I hope you @joshwalawender don't mind that git now thinks I applied them?