Added NAFE #24

MinaSGorgi · 2019-04-08T04:54:32Z

Description

This is a complete working implementation of the NAFE filter. Only missing feature is setting default values for input and output margins according to image contrast which is provided by original version. Otherwise, it supports all features including default parameters for different wavelengths. Original code is not open-sourced, hence can't compare it code-wise and thorough testing against original code results is still in progress. Differences between this implementation and other available open-source version are mainly implementation-wise. The only non-trivial difference is using constant noise removal sigma.

An output sample: (400x400) in ~10 mins

Creating this PR for feedback. @nabobalis @dpshelio

Fixes #6

pep8speaks · 2019-04-08T04:54:35Z

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

In the file sunkit_image/nafe.py:

Line 44:20: E251 unexpected spaces around keyword / parameter equals
Line 92:101: E501 line too long (142 > 100 characters)
Line 214:9: E731 do not assign a lambda expression, use a def

Comment last updated at 2019-05-04 01:12:18 UTC

ghost · 2019-04-08T04:54:38Z

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

nabobalis · 2019-04-08T05:55:52Z

Thanks for this, I will have a proper review later on but any chance you can change your image to focus on the edges of the Sun?

I'd like to see the output on a patch of loop structures nearer the edge.

MinaSGorgi · 2019-04-08T06:44:53Z

I get what you want but current sample images I am working with doesn't have such feature. If you can point a specific image in mind I can definitely provide that. This is the best I have currently.

NB: I currently remove all pixels out of ROI while instead I should keep them but evaluate new pixels for only the ones inside ROI. Will change locally for now.

nabobalis · 2019-04-08T08:07:12Z

That region works, are you doing any extra normalization to plot the image?

sunkit_image/nafe.py

nabobalis · 2019-04-08T08:09:50Z

sunkit_image/nafe.py

+    return new_margin[0] + (new_margin[1] - new_margin[0]) * (((old_value - old_margin[0]) / (old_margin[1] - old_margin[0])) ** (1.0 / power))
+
+
+class _Nafe():


I am not sure I am in favour of having a class here, is it not possible to avoid this?

Probably yes. Will apply or report if it's not.

I am curious why would a class not be preferred. I come from a C++ background so that's why it seemed the way to go for me.

I guess I don't understand why we need a class here.

Basically used for keeping reference of the shared variables across the processes pool. Just making the code more readable and the mapping function cleaner.

This: pool.imap(nafe_instance.transform, indices), total=map_data.size) is what I mean. I need to use imap because the order of the results matter since they are pixels values. AFAIK the only other way to pass the arguments to imap would be creating an iterable whose elements will mainly be in the form (row, col, some constant arguments). I thought since they are constant arguments this seemed a typical case for using a class.

Well if it's the best way to do it, it can stay.

Will keep for now then.

I'm ok with the class, but why not having it all as one? I mean to include the nafe function as to execute it?

I have no preference but current implementation doesn't require a class anymore.

nabobalis · 2019-04-08T08:11:00Z

sunkit_image/nafe.py

+    axes[0].set_title('Original Image')
+    axes[1].imshow(nafe_image, cmap='gray')
+    axes[1].set_title('Enhanced Image')
+    plt.show()


This can all go. We don't want to be able to call the function like this. All those arser.add_argument( should the parameters in the corresponding doc strings.

This could be turned into a seperate gallery example.

It's basically for debugging purposes but will apply for final PR.

Though I agree with @nabobalis and probably this wouldn't be the place. I imagine sunkit-image could have some command line programs that run some of these functions. This would need, however some extra additions to the setup.py.

Will remove from file for now and investigate where to migrate after finishing testing.

nabobalis · 2019-04-08T08:12:18Z

sunkit_image/nafe.py

+def nafe(input_map, imargin=None, gamma=None, omargin=None, nafe=False,
+         nafe_weight=None, roi_p=None, roi_f=None, nproc=None, hbins=None,
+         nrs=15, gb=0.3, lb=0.7, gw=0.3, lw=0.7, n=129,
+         c=[1 for _ in range(12)]):


This just creates a list of 1 that is 12 long?

I wouldn't put a loop as part of the arguments. Rather None and then check it later and create the ones. Then, maybe this is more readable:

c = np.ones(12)

Will apply the ones one.

nabobalis · 2019-04-08T08:16:00Z

sunkit_image/nafe.py

+    ----------
+    input_map : `~sunpy.map.sources.sdo.AIAMap`
+        The original input image to be enhanced.
+


You should remove these line breaks between parameters.

Will apply.

nabobalis · 2019-04-08T08:16:48Z

sunkit_image/nafe.py

+    input_map : `~sunpy.map.sources.sdo.AIAMap`
+        The original input image to be enhanced.
+
+    imargin: `~tuple`


The ~ shorterns the name in the sphinx build, you only want it for long import. For the basic python types, you don't need them.

Will apply.

nabobalis · 2019-04-08T08:17:09Z

sunkit_image/nafe.py

+
+    Parameters
+    ----------
+    input_map : `~sunpy.map.sources.sdo.AIAMap`


This will only work on AIAMap and not any Map?

For current implementation: anything with .meta["wavelnth"] and .data

Should I change this to take a numpy array instead and an optional wavelength argument? The original program uses the wavelength to calculate the optimal values for some other parameters. I can assign the default values for these parameters instead if not given a wavelength.

That would be a good idea.

Will apply.

I think I prefer a map instead of a numpy array as an input, as it would be the most used. If I have a numpy array, then I can create a map instead to run it through here.

I thought it would be easier to pass the map.data rather than possibly having to create a map from a numpy array. Again I have no preference. If a map would be sent I will check if its header contains info about the wavelength else I will use default value.

MinaSGorgi · 2019-04-08T13:06:48Z

Thanks @nabobalis for your review and feedback. Regarding extra normalization, nothing not mentioned in the algorithm.

nabobalis · 2019-04-08T13:09:32Z

In one of the examples in the SunPy Gallery, we apply just a log scale to the intensity. Any chance for the second example, you apply that to both images?

MinaSGorgi · 2019-04-08T18:52:15Z

Applying logNorm on the last 2 images gives:

sunkit_image/nafe.py

nabobalis · 2019-04-09T10:34:09Z

Can you do a comparison with the other Python version?

Also with the original version, are any of the examples got a timestamp we can trace? If so, we could download the original data and do a more "direct" comparison.

nabobalis · 2019-04-09T10:42:29Z

Applying logNorm on the last 2 images gives:

Thanks for that! Maybe it's just my old age, but I can't really spot any difference between these two images with the scaling now the same.

dpshelio

Great work @MinaSGorgy
This still needs tests!

sunkit_image/nafe.py

dpshelio · 2019-04-13T19:08:48Z

sunkit_image/nafe.py

+def nafe(input_map, imargin=None, gamma=None, omargin=None, nafe=False,
+         nafe_weight=None, roi_p=None, roi_f=None, nproc=None, hbins=None,
+         nrs=15, gb=0.3, lb=0.7, gw=0.3, lw=0.7, n=129,
+         c=[1 for _ in range(12)]):


I wouldn't put a loop as part of the arguments. Rather None and then check it later and create the ones. Then, maybe this is more readable:

c = np.ones(12)

dpshelio · 2019-04-13T19:10:09Z

sunkit_image/nafe.py

+
+def nafe(input_map, imargin=None, gamma=None, omargin=None, nafe=False,
+         nafe_weight=None, roi_p=None, roi_f=None, nproc=None, hbins=None,
+         nrs=15, gb=0.3, lb=0.7, gw=0.3, lw=0.7, n=129,


I dislike very short variable names. nrs, gb, ...? I like to look at the options and know what these parameters are, if I don't need to read the documentation, then the better, that means is self-explanatory.

Ideally the user would never change these parameters values. Some of them even aren't mentioned in the paper but I adopted them from the original software implemented by the paper's author. In this software they are even read from a file containing fine tuned default values. The author exposes changing them through changing the file, so I thought I would rather expose them as functions arguments. I totally agree with having self-explanatory variables as I normally go for but I took these as named in the mentioned file. Of course I can change if you find it better to.

sunkit_image/nafe.py

dpshelio · 2019-04-13T20:04:08Z

sunkit_image/nafe.py

+    return new_margin[0] + (new_margin[1] - new_margin[0]) * (((old_value - old_margin[0]) / (old_margin[1] - old_margin[0])) ** (1.0 / power))
+
+
+class _Nafe():


I'm ok with the class, but why not having it all as one? I mean to include the nafe function as to execute it?

dpshelio · 2019-04-13T20:07:37Z

sunkit_image/nafe.py

+    axes[0].set_title('Original Image')
+    axes[1].imshow(nafe_image, cmap='gray')
+    axes[1].set_title('Enhanced Image')
+    plt.show()


Though I agree with @nabobalis and probably this wouldn't be the place. I imagine sunkit-image could have some command line programs that run some of these functions. This would need, however some extra additions to the setup.py.

sunkit_image/nafe.py

nabobalis · 2019-04-13T20:19:58Z

Thanks for the review @dpshelio

MinaSGorgi · 2019-04-13T21:11:56Z

Thanks @dpshelio for your review 😄 I think we mainly need to decide on using a map or numpy array as input. Meanwhile I am working on testing. Will reopen a PR once all changes are applied and testing is complete.

dpshelio · 2019-04-14T13:55:15Z

No need to re-open the PR, you can add to this, bit by bit. I've just labelled as WIP as you did on the title.

MinaSGorgi · 2019-05-03T15:10:28Z

Sorry for late reply, last two weeks have been TOO hectic.

Applied all changes requested mainly:

Removed NAFE class and improved multiprocessing.
Changed to accepting a sunpy.map.Map and returning same.[1]
Removed custom sub-mapping.
Changed printing warnings to Python warn.
Added missing documentation.
Added testing.[2]

[1] Ideally nafe should support ROI but it's removed for now since some information is currently missing.
[2] Breaks CI for introducing PyTest. Missing main test comparing module output to original software because current AIA maps in samples are rejected by original software(says it treats them as colored images which aren't supported) and I am facing trouble getting AIA images due to some internal server errors.

TODO:

Add missing test which mainly would be a very small portion of an AIA map(something like 300x300) to run fast and just test calculations.
Support ROI (future work)

Finally for comparing results, the only maps I currently have that are accepted by the original software are 4k*4k which would take quite some time to process. Cropping them with SunPy produces rejected maps for some reason.

@nabobalis @dpshelio

nabobalis · 2019-05-03T20:11:35Z

Thanks for the update. I will have to review it in more detail later on.

Just a few comments tho:

I don't see why adding pytest would break any of the CI.

I think ROI would be nice but not really important. People should probably pass in submaps if they want a smaller area.

I did try to load in some data I downloaded and NAFE would not accept it, I don't know why tho.

Can you not crop a region within the NAFE windows program? Should hopefully crop the 4k image down a bit.

I have to reiterate tho, if the before and after images are the same after we apply some normalization, has the code worked correctly?

MinaSGorgi · 2019-05-03T20:35:13Z

Welcome @nabobalis . Take your time.

Regarding CI break: I thought I saw something suggesting that in the error log but seems there isn't. Will investigate this. Any hints though?

Regarding the ROI: the thing is the original software most probably doesn't discard the out of ROI pixels. ie it doesn't really crop the image but just calculates new values for the ROI pixels including in the calculations the out of ROI neighbor pixels. It even does this with a ratio which we can change in the ini file but I am not 100% sure it's linear. Hence only full size images would provide reasonable comparison.

Regarding before and after images: if I am getting it correctly that you are even questioning the benefit of the filter itself against simpler normalization filters, this I can test using original software.

nabobalis · 2019-05-03T20:47:55Z

I'm not sure what to hint about, the CI has all passed bar the install one. That is a known fault and I need to fix it separately.

Yes I guess I'm questing the need for this filter if it's just the same as normalizing it with a simple function. Which makes me suspect that if the original method is "correct", then there is a mistake somewhere in the version here.

nabobalis reviewed Apr 8, 2019

View reviewed changes

sunkit_image/nafe.py Show resolved Hide resolved

nabobalis reviewed Apr 8, 2019

View reviewed changes

sunkit_image/nafe.py Show resolved Hide resolved

nabobalis marked this pull request as ready for review April 9, 2019 10:45

dpshelio suggested changes Apr 13, 2019

View reviewed changes

dpshelio added the [WIP] label Apr 14, 2019

MinaSGorgi force-pushed the master branch 3 times, most recently from 3b0edb7 to 3d8de26 Compare May 3, 2019 14:49

MinaSGorgi changed the title ~~Added nafe.py [WIP]~~ Added NAFE May 3, 2019

nabobalis added the Feature Request label May 3, 2019

nabobalis added this to the v0.1.0 milestone May 3, 2019

MinaSGorgi force-pushed the master branch 6 times, most recently from 97f6b59 to 9a7ce1e Compare May 3, 2019 23:10

Added nafe.py

cbacdb6

MinaSGorgi force-pushed the master branch from 9a7ce1e to cbacdb6 Compare May 4, 2019 01:12

nabobalis mentioned this pull request Jul 1, 2019

Noise Adaptive Fuzzy Equalization method (NAFE) #6

Open

6 tasks

nabobalis added the Needs Adoption label Aug 22, 2019

nabobalis modified the milestones: v0.1.0, v0.2.0 Nov 17, 2019

nabobalis closed this Nov 28, 2020

nabobalis removed this from the v0.2.0 milestone Nov 28, 2020

		return new_margin[0] + (new_margin[1] - new_margin[0]) * (((old_value - old_margin[0]) / (old_margin[1] - old_margin[0])) ** (1.0 / power))


		class _Nafe():

Added NAFE #24

Added NAFE #24

Conversation

MinaSGorgi commented Apr 8, 2019 • edited

Description

pep8speaks commented Apr 8, 2019 • edited

Comment last updated at 2019-05-04 01:12:18 UTC

ghost commented Apr 8, 2019 • edited by ghost

nabobalis commented Apr 8, 2019 • edited

MinaSGorgi commented Apr 8, 2019

nabobalis commented Apr 8, 2019

nabobalis Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinaSGorgi commented Apr 8, 2019

nabobalis commented Apr 8, 2019

MinaSGorgi commented Apr 8, 2019

nabobalis commented Apr 9, 2019 • edited

nabobalis commented Apr 9, 2019

dpshelio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Apr 13, 2019

MinaSGorgi commented Apr 13, 2019

dpshelio commented Apr 14, 2019

MinaSGorgi commented May 3, 2019 • edited

nabobalis commented May 3, 2019

MinaSGorgi commented May 3, 2019

nabobalis commented May 3, 2019

MinaSGorgi commented Apr 8, 2019 •

edited

pep8speaks commented Apr 8, 2019 •

edited

ghost commented Apr 8, 2019 •

edited by ghost

nabobalis commented Apr 8, 2019 •

edited

nabobalis Apr 8, 2019 •

edited

nabobalis commented Apr 9, 2019 •

edited

MinaSGorgi commented May 3, 2019 •

edited