Skip to content
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

Added normalized_epsilon as a parameter to convolve_fft #5177

Closed

Conversation

samverstocken
Copy link
Contributor

This allows the user of convolve_fft to choose the upper limit of deviation between the kernel sum and unity, so that it can for example be adjusted for the specific floating-point precision of the kernel array

This allows the user of convolve_fft to choose the upper limit of deviation between the kernel sum and unity, so that it can for example be adjusted for the specific floating-point precision of the kernel array
@@ -312,6 +312,9 @@ def convolve_fft(array, kernel, boundary='fill', fill_value=0, crop=True,
256.
quiet : bool, optional
Silence warning message about NaN interpolation
normalized_epsilon: float, optional
The upper limit on the deviation of the kernel sum with respect to unity.
Will be checked only if normalize_kernel is False.
Copy link
Member

@pllim pllim Jul 14, 2016

Choose a reason for hiding this comment

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

Add `` around normalize_kernel to mark it as another keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if a parameter is optional it is custom to provide or hint at the default value. Something like Default is "1e-8". (or defaults to xxx, ...)

Another thing that could be added is a sphinx directive when the parameter was added, something like: .. versionadded:: 1.3 (or 1.2.x).

This reminds me: This PR needs a Changelog entry (home directory /Changes.rst) for the appropriate version/subpackage under "API changes" (or "New features").

Copy link
Member

Choose a reason for hiding this comment

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

This is more of an API change.

@pllim
Copy link
Member

pllim commented Jul 14, 2016

This looks uncontroversial enough to me, but let's see what the sub-package maintainer(s) say. Still needs a change log.

@MSeifert04
Copy link
Contributor

I'm not sure if it's appropriate here but numpy has a finfo() function to get the smallest epsilon:

import numpy as np

def get_epsilon(arr):
    return np.finfo(arr.dtype).eps

>>> get_epsilon(np.array([1,2,3], dtype=np.float32))
1.1920929e-07
>>> get_epsilon(np.array([1,2,3], dtype=np.float64))
2.2204460492503131e-16

But that's only the minimal epsilon for the number 1, but it could provide at least an order of magnitude estimate.

@samverstocken
Copy link
Contributor Author

@MSeifert04 Thanks for showing that finfo() function, good to know.
@pllim @MSeifert04 I will try to improve the PR with your suggestions.

@@ -226,8 +226,8 @@ def convolve(array, kernel, boundary='fill', fill_value=0.,
def convolve_fft(array, kernel, boundary='fill', fill_value=0, crop=True,
return_fft=False, fft_pad=None, psf_pad=None,
interpolate_nan=False, quiet=False, ignore_edge_zeros=False,
min_wt=0.0, normalize_kernel=False, allow_huge=False,
fftn=np.fft.fftn, ifftn=np.fft.ifftn,
min_wt=0.0, normalize_kernel=False, normalized_epsilon=1e-8,
Copy link
Contributor

Choose a reason for hiding this comment

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

This line contains a trailing whitespace (at the end of the line). That's why your first build failied during the PEP-8 check.

@eteq eteq modified the milestones: v2.0.0, v1.3.0 Dec 18, 2016
@eteq
Copy link
Member

eteq commented Dec 18, 2016

Pushing this to 2.0 as it appears @astrofrog's comments have not yet been resolved and the branch has conflits.

@samverstocken
Copy link
Contributor Author

I'm sorry but I currently don't have the time to look at his again.. Alas, not paid to do programming..

@pllim
Copy link
Member

pllim commented Dec 24, 2016

Is anyone interested to take over? If not, I guess we can close this.

@pllim pllim added the Close? label Dec 24, 2016
@mwcraig
Copy link
Member

mwcraig commented Dec 24, 2016

Seems pretty close to done, I could work on it Monday. May have to open a new PR with these changes cherry-picked if a force push doesn't work....

@mwcraig
Copy link
Member

mwcraig commented Dec 26, 2016

Closing in favor of #5649, which is this rebased, and with the comments made here addessed.

@mwcraig mwcraig closed this Dec 26, 2016
keflavich added a commit that referenced this pull request Jun 19, 2017
Added normalized_epsilon as a parameter to convolve_fft (replaces #5177)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants