Skip to content

Implemented the Fourier Normalizing Radial Gradient Filter#17

Merged
nabobalis merged 55 commits intosunpy:masterfrom
vatch123:master
Jun 10, 2019
Merged

Implemented the Fourier Normalizing Radial Gradient Filter#17
nabobalis merged 55 commits intosunpy:masterfrom
vatch123:master

Conversation

@vatch123
Copy link
Contributor

@vatch123 vatch123 commented Mar 3, 2019

Description

It implements Fourier normalizing-radial-graded filter (FNRGF) as stated here.
The changes made are -

  • Imported sunpy.coordinates.frames
  • Changes in the normalizing_radial_gradient_function
    • Changed the default intensity_summary method from numpy.mean to numpy.nanmean
    • Changed the keyword arguments from None to empty dictionary because it raised an error at runtime stating the types did not match
  • Implemented fourier_normalizing_radial_gradient function.
    • The function seems to work fine for different values of order and ratio_mix. Though I am unable to guess what values of parameters are best suited.
    • The default values of few parameters except for the ratio_mix is set by taking inspiration from the doctoral thesis.
    • Some initial tests are added, which needs further improvement.

TODO:

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

Fixes #7

@nabobalis

This comment has been minimized.

@pep8speaks
Copy link

pep8speaks commented Mar 7, 2019

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

Line 6:101: E501 line too long (105 > 100 characters)

Line 315:101: E501 line too long (104 > 100 characters)
Line 316:101: E501 line too long (105 > 100 characters)
Line 319:101: E501 line too long (105 > 100 characters)
Line 324:101: E501 line too long (106 > 100 characters)
Line 325:101: E501 line too long (106 > 100 characters)
Line 333:101: E501 line too long (113 > 100 characters)
Line 336:101: E501 line too long (113 > 100 characters)
Line 344:101: E501 line too long (105 > 100 characters)
Line 345:101: E501 line too long (106 > 100 characters)
Line 346:101: E501 line too long (113 > 100 characters)
Line 358:50: E203 whitespace before ':'
Line 383:101: E501 line too long (106 > 100 characters)
Line 384:101: E501 line too long (101 > 100 characters)
Line 385:101: E501 line too long (101 > 100 characters)
Line 386:101: E501 line too long (104 > 100 characters)
Line 387:101: E501 line too long (105 > 100 characters)
Line 388:101: E501 line too long (105 > 100 characters)
Line 401:101: E501 line too long (106 > 100 characters)
Line 422:101: E501 line too long (104 > 100 characters)

Comment last updated at 2019-06-10 14:50:17 UTC

@ghost
Copy link

ghost commented Mar 7, 2019

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

@nabobalis nabobalis added this to the v0.1.0 milestone Mar 7, 2019
@vatch123 vatch123 changed the title Implemented the Fourier Normailizing Radial Gradient Filter Implemented the Fourier Normalizing Radial Gradient Filter Mar 7, 2019
@nabobalis nabobalis added [Review] and removed [WIP] labels Jun 10, 2019
@nabobalis
Copy link
Member

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

@vatch123
Copy link
Contributor Author

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

Yes, it will be my final commit after everything is done.

This is a helper function to Fourier Normalizing Radial Gradient Filter
(`sunkit_image.radial.fnrgf`).

This function sets the attenuation coefficients in the one of the following two manners-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This function sets the attenuation coefficients in the one of the following two manners-
This function sets the attenuation coefficients in the one of the following two manners:


# The NRGF filtered map is plotted.
# The image seems a little washed out so you may need to change some plotting settings
# for a clearer output.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to change the plotting values to prevent the image from being so washed out?

I do wonder if something is wrong with this version of the algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, which values?
But I don't think the algorithm does anything other than what was intended

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, some of the plotting values?

@nabobalis
Copy link
Member

Codecov seems to suggest that these two lines are not triggered by the tests:

if cutoff > (order + 1):
    raise ValueError("cutoff cannot be greater than order + 1")

https://codecov.io/gh/sunpy/sunkit-image/compare/118447604c51a28de4d9390511f5d1787f5d8a1d...72b95663cdb182a34f9c214b9c5f3f73618bd7b9/diff#D10-355

and

if order < 1:
     raise ValueError("Minimum value of order is 1")

https://codecov.io/gh/sunpy/sunkit-image/compare/118447604c51a28de4d9390511f5d1787f5d8a1d...72b95663cdb182a34f9c214b9c5f3f73618bd7b9/diff#D10-448

Could you write a test to trigger those warnings and check the messages are correct?

@nabobalis
Copy link
Member

This is very close now.

@vatch123
Copy link
Contributor Author

I hope its done now!!

@nabobalis
Copy link
Member

Yes I think that addresses everything!

@nabobalis nabobalis merged commit 19f2743 into sunpy:master Jun 10, 2019
@nabobalis
Copy link
Member

Thanks @vatch123

vatch123 added a commit to vatch123/sunkit-image that referenced this pull request Jun 10, 2019
Implemented the Fourier Normalizing Radial Gradient Filter (sunpy#17)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Request New feature wanted. [Review]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fourier normalizing-radial-graded filter (FNRGF)

4 participants