-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Support spectra & noise propagation #32
Conversation
Regarding this:
There's actually no reason to support subtracting a constant background internally, is there? You were probably just proposing to get rid of |
Does this look like what you had in mind, @cmccully? I have not added a noise argument yet, but this removes the |
For completeness, I've had a go at adding a
|
I'll push what I did so you can see it. We can always revert that commit if needed... I'm not returning a noise image here, but I suppose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overall, this looks really helpful. I made a few minor comments about the implementation. I'm going to merge #41 before this so you can rebase to that if you would like.
Input data array that will be used for cosmic ray detection. This | ||
should include the sky background (or a mean background level, added | ||
back in after sky subtraction), so that noise can be estimated | ||
correctly from the data values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe say unless the var array is also provided.
|
||
var : float numpy array, optional | ||
A pre-determined estimate of the data variance (ie. noise squared) in | ||
each pixel, generated by previous processing of ``indat``. If provided, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might remove "generated by previous processing of indat
".
each pixel, generated by previous processing of ``indat``. If provided, | ||
this is used in place of an internal noise model based on ``indat``, | ||
``gain`` and ``readnoise``. This still gets median filtered and cleaned | ||
internally, to estimate what the noise in each pixel *would* be in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the median filtering done in place? We probably shouldn't. We should probably make a copy of the array if we are not already.
internally, to estimate what the noise in each pixel *would* be in the | ||
absence of cosmic rays, but without removing ``bkg`` temporarily (a | ||
difference that could lead to less accurate results than the default | ||
noise model close to fine structure). This argument should be provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it just make sense to take the masked median of the variance array? Or just replace the masked regions with the median filter of the var array? This way you wouldn't have to worry about the bkg stuff.
noise model close to fine structure). This argument should be provided | ||
for correct results if ``bkg`` does not include all signal that has | ||
previously been subtracted from ``indat`` (other than electronic bias). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to note that var is expected to be in adu like bkg and indat (I really should have named that variable better).
if var is not None: | ||
goodvar = np.empty_like(gooddata, order='c') | ||
goodvar[:] = var[np.logical_not(mask)] | ||
var_level = median(goodvar, len(goodvar)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want an else var_level = background_level?
# order to estimate the noise accurately (first saving a copy if | ||
# using the same median to replace bad values): | ||
if cleantype == 'median': | ||
m5_nobkg = m5 if bkg is None else m5.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just replace this by working on the cleanvar below? See comment below.
|
||
if cleantype != 'median': | ||
if var is None: | ||
noise = np.sqrt(m5 + readnoise * readnoise) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using m5 in the noise, why don't you use the 5x5 median of cleanvar. Basically calculate one m5 for the cleanarr for the cr detection and one m5 for the noise calculation. Then you don't have to worry about adding back in bkg etc.
del m5 | ||
cleanarr[crinds] = m5_nobkg[crinds] | ||
del m5_nobkg | ||
if var is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, I think you can have a noise and just median of the cleanarr which would simplify this whole block considerably.
# Subtract the input sky model, if applicable. | ||
if bkg is not None: | ||
cleanarr -= bkg | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should happen much later in the function. The background needs to be included when you set cleanvar = cleanarr
at line 221.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The background for the noise estimate gets restored again at L303, which needs to happen after the median filtering; see recent discussion at jehturner#1 for further explanation.
Rebased as requested. |
Not sure what is going on with the first, Python 2.7 build test? It seems to install Python 3.7 with Conda and then fail, but I don't think it's relevant to this PR. |
Before answering your detailed suggestions (thanks), I'd like to point out that I was leaning towards reverting my last commit (see comment of 16 April). Input variance was something we discussed on Slack that seemed like a good idea at the time, but once I actually implemented |
In our meeting earlier this week, @cmccully mentioned that he's keen to retain variance input as an option because he has a use case (I think for echelle data) where prior processing produces a non-trivial correspondence betweeen The good news is that I think the code here already does most of the duplicate processing necessary to use input variance accurately and is "only" missing subtraction/restoration of the background structure (as for The problem is simply how to go about subtracting the background from the variance. Unless the only processing done prior to cosmic ray removal is bias subtraction and maybe stacking (in which case you don't really need So what are your thoughts on how exactly |
Can you rebase this into master @jehturner ? |
Rebase from |
…rappy's predecessor, lacosmicx (producing identical results in a quick test).
…d background internally instead of taking pre-subtracted input, as discussed with cmucully on the astropyspectroscopy slack channel in April.
…noise estimates, in place of the internal noise model. (May want some tweaking, eg. to protect bkg structure for noise estimates too??)
That was suspiciously easy... Anyway, the diff is just 1 file now. |
Codecov Report
@@ Coverage Diff @@
## master #32 +/- ##
==========================================
- Coverage 94.88% 92.94% -1.94%
==========================================
Files 7 7
Lines 1016 1049 +33
Branches 53 53
==========================================
+ Hits 964 975 +11
- Misses 52 74 +22
Continue to review full report at Codecov.
|
This is the same PR as cmccully/lacosmicx#1, rebased on astroscrappy, so we can close that old one. It doesn't yet implement the intended behaviour summarized in that PR, so do not merge yet.