DM-54011 Give option to scale noise in RGB images #1242
Conversation
having backgrounds that go to pure black in images, especially when pixels are negative can be visualy unappealing. Add an option to bring the noise floor up by a configurable amount.
| # Skip processing if sigma is invalid | ||
| if not np.isfinite(sigma): | ||
| processed_arrays.append(working) | ||
| continue |
There was a problem hiding this comment.
This if block can probably be moved to the top of the for loop, to avoid the need in making a copy of the data array if sigma is invalid.
There was a problem hiding this comment.
oh no, I remember now, I did this on purpose. I didn't want some of the returns to be copies and some to not be, because then it might be hard to reason about what you can modify without side effects and what you can't. However later I went back and added the any check that returns without copying. So now I am torn between what I should do, make it all conceptually take ownership (by making copies), make it all operate in place, or accept for performance this is an internal operation and do the optimal performance thing, which I guess would be modify in place... I'll think about that. Feel free to weight in if you have thoughts.
|
|
||
| processed_arrays.append(working) | ||
|
|
||
| return processed_arrays |
There was a problem hiding this comment.
If you exit early above, you return a tuple. Here you're returning a list. It might be cleaner to always return the same thing (either tuple(...) here, or list above).
There was a problem hiding this comment.
You're not wrong here, but the efficiency person in me cries for this. Thanks for pointing it out though I'll think about it.
| sigma_ratio = min_sig / sigma | ||
|
|
||
| # Apply adjustment to qualifying values | ||
| working[lower_pos] = array[lower_pos] * sigma_ratio * factor + min_sig * factor |
There was a problem hiding this comment.
I don't understand this line. It has two parts as I see it:
array[lower_pos] * sigma_ratio * factor
This scales any data arrays with high sigma down to the sigma of the lowest sigma array. For the lowest sigma array (assuming factor=1), this is effectively a no-op.
then:
+ min_sig * factor
This does not match sigmas; it bulk shifts your pixel values upwards for all pixels, even the lowest sigma data array. I wonder if that is the intent here, and if it will always work? The halfnorm.fit operation also fits for the mean, which we're not accounting for here. If the mean is far from zero, this + min_sig correction might be insufficient.
Further, we're only applying the correction to values in the lower_pos mask, which is a subset of pixels. This will leave us with an overlapping distribution of flux for pixels in the range [sigma, sigma+min_sig], and a step-function mis-match in noise at around the transition.
There was a problem hiding this comment.
We spoke about this a bit in person. You are correct that there can be a tweak to deal with non zero means. And that was actually one of the primary points of the function, which means I need to rename the function to make this more clear.
There was a problem hiding this comment.
I will note it dose match sigmas, assuming zero centered, that is what the sigma ratio is doing.
|
|
||
| # Return infinity if no negative values found | ||
| if values_neg.size == 0: | ||
| return np.inf |
There was a problem hiding this comment.
Out of curiosity, is there a reason you decided to return Inf instead of, e.g., NaN?
There was a problem hiding this comment.
Inf are still comparable, so you can still do min, max, etc. NaN is not comparable (well the operators work but it does not really behave like a number)
having backgrounds that go to pure black in images, especially when pixels are negative can be visualy unappealing. Add an option to bring the noise floor up by a configurable amount.