Implemented Multi-scale Gaussian Normalization

[vatch123]

Description

• Contains both @Cadair and @ebuchlin code. As both the implementations are accurate with minor differences and both provide visually acceptable outputs.
• Cadair's code has been modified as mentioned in this comment.
• Though Eric's code has minor differences from the paper as pointed out by him here.
• Have a look at this gist to see the differences in the output of both.

TODO:

• Review of PR
• ~100% test line coverage
• Code formatted/linted.
• Example and documentation is detailed.

Fixes #1

[pep8speaks]

Hello @vatch123! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file sunkit_image/enhance.py:

Line 32:101: E501 line too long (108 > 100 characters)
Line 33:101: E501 line too long (108 > 100 characters)
Line 42:101: E501 line too long (104 > 100 characters)
Line 61:101: E501 line too long (103 > 100 characters)

Comment last updated at 2019-06-20 15:37:58 UTC

[ghost]

Thanks for the pull request @vatch123! Everything looks great!

[nabobalis]

I think the main point is that we don't want to have both versions in our code but one unified version which we can switch to (maybe with a keyword) if we want.

[vatch123]

I think the main point is that we don't want to have both versions in our code but one unified version which we can switch to (maybe with a keyword) if we want.

Then it is better that we choose one because both are essentially the same. The only difference is that one has some weights and the other doesn't (which is equivalent to setting the weights 1).

[nabobalis]

The output doesn't look similar. So what is causing that?

[vatch123]

The output doesn't look similar. So what is causing that?

• The image is normalized before being processed which is then used later as mentioned in this comment.
• The weights g_i as mentioned in the paper have not been used.
• The calculated values after applying the transform has been again brought between (0,1) from (-pi/2, pi/2). This is nowhere mentioned in the paper.

Please have a look at this comment for greater details as how the code varies from the paper.

I think the main point is that we don't want to have both versions in our code but one unified version which we can switch to (maybe with a keyword) if we want.

Then working on Cadair's code is better because now it is exactly similar to the paper and gives better visual output too.

[ebuchlin]

About visual output comparison (as seen on the gist): the output depends on the input parameters, which are not specified in the gist. I guess that they are the default parameters, but the default parameters are not the same for mgn1() and mgn2() in enhance.py.

This can be seen immediately for sigma, but it is also the case for the global weights: the defaults h=0.7 for mgn1() and weight=0.3 for mgn2() are not equivalent (although 0.3 == 1 - 0.7), because of the arctan normalization from [-π/2, π/2] to [0,1] in the end of mgn2(). I would say (untested) that the equivalency would be given by 1 - h == weight / pi, that is weight = .3 * pi (about .94) for h = 0.7. The fact that .94 is much closer to 1 than 0.3 would explain why the (gamma-corrected) background image dominates in both "Eric's Outputs" in @vatch123's gist, while the pure Gaussian normalization dominates in both "Cadair's Outputs" in the gist.

IMHO the definition of "weight" (i.e. with arctan normalization to [0,1]) is much more meaningful that that of "h".
This is almost independent of the implementation itself (I think both are fine, and I appreciate the speed and memory optimizations made for mgn1()).

[vatch123]

Yes, the default parameters in both the implementation are different. But after discussion with @nabobalis and @wafels we have decided to only have mgn1() just because it is an exact replica of the paper and the parameters also follow the paper.

[nabobalis]

@ebuchlin Would it be possible for you to do another review for us?

[nabobalis]

Could you run black, isort and docformater on your PR?

[codecov]

Codecov Report

Merging #30 into master will decrease coverage by 5.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
- Coverage   82.72%   77.13%   -5.59%     
==========================================
  Files          13       13              
  Lines         573      433     -140     
==========================================
- Hits          474      334     -140     
  Misses         99       99

Impacted Files	Coverage Δ
sunkit_image/utils/tests/test_utils.py	85.33% <ø> (-0.2%)	⬇️
sunkit_image/enhance.py	100% <100%> (ø)
sunkit_image/tests/test_enhance.py	100% <100%> (ø)
sunkit_image/utils/utils.py	66.66% <0%> (-30.96%)	⬇️
sunkit_image/radial.py
sunkit_image/tests/test_radial.py
sunkit_image/conftest.py	62.31% <0%> (+0.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19f2743...3386e12. Read the comment docs.

[vatch123]

This is done too, I guess

[nabobalis]

Did you run the linter/formaters on the code?

I think it is ready but I would like either @ebuchlin or @Cadair to review it once more (before I merge this) since they helped here.

Until that happens, we should go back to working on the OCCULT-2 code!

[vatch123]

Okay, I will begin on it again from tomorrow.
Yes I ran the linters/formaters

[ebuchlin]

Here are some comments after reading the code again. Disclaimer: I haven't tried to run the code (I don't know how to do with the code of a PR).

[ebuchlin]

I'm a bit uncomfortable with that. The paper is explicitly for EUV data only with normally only positive values, but I don't see where it is required or enforced. Actually there are negative values in low-signal, noisy areas of SDO/AIA images, and these are as meaningful as the higher-value pixels in these low-signal areas, there is no reason to clip them to 0. Furthermore, as the code does not tell explicitly that it is for EUV images only, some users would perhaps like to try with e.g. magnetogram Bz data, which would probably be fine except that the gamma transform of the original data would not be the best choice for representing such data.

[nabobalis]

If we set a keyword that makes this stage optional? Would that be an ok compromise?

[nabobalis]

Thanks for the review @ebuchlin! If you want to run the code, the most straightforward way would be to clone the branch this code is on by doing git clone -b mgn https://github.com/vatch123/sunkit-image.git then either installing the package or I think if the deps are just installed, you should be able to run the file (maybe?).

[ebuchlin]

Thanks for the review @ebuchlin! If you want to run the code, the most straightforward way would be to clone the branch this code is on by doing git clone -b mgn https://github.com/vatch123/sunkit-image.git then either installing the package or I think if the deps are just installed, you should be able to run the file (maybe?).

Thanks, I could just run the example file, and the output was as expected.

[nabobalis]

Thanks @vatch123 and @ebuchlin for your reviews!