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

Remove side-effects from convolution.convolve_fft #8107

Closed
jamienoss opened this issue Nov 10, 2018 · 6 comments
Closed

Remove side-effects from convolution.convolve_fft #8107

jamienoss opened this issue Nov 10, 2018 · 6 comments

Comments

@jamienoss
Copy link
Contributor

[src]

# restore NaNs in original image (they were modified inplace earlier)
# We don't have to worry about masked arrays - if input was masked, it was
# copied
array[nanmaskarray] = np.nan
kernel[nanmaskkernel] = np.nan
@jamienoss
Copy link
Contributor Author

jamienoss commented Nov 10, 2018

In fact, this is not even correct! nanmaskarray is computed as ~np.isfinite(array) (src), and therefore any inf in the input array or kernel will be incorrectly replaced by NaN.

@pulkit6559
Copy link

pulkit6559 commented Nov 12, 2018

I'm new to astropy and a beginner, is it okay if i work on this?

@pllim
Copy link
Member

pllim commented Nov 12, 2018

@pulkit6559 , please checkout the issues labeled as "good first issue" first. Thank you for your interest!

@jamienoss
Copy link
Contributor Author

jamienoss commented Nov 12, 2018

@pulkit6559 I wasn't necessarily intending to work on this. The soln. mainly involves correctly copying the input arrays only when necessary. See #7503

One caveat, I wouldn't start until ##8114 is resolved, which I'm currently working on. convolve_fft() will be refactored in which case starting now might mean doing twice the work for you. Unless you finish first in which case it's twice for me, but I'm cool with that 😉

Also, I should note that I am not a maintainer of this package so have no actual official say when it comes to workload distribution. That being said, it is open src and, canonically, everyone is "free" to work on whatever they want. The only caveat is that someone else might beat you to the punch and/or do a better job. Either way, you are free to try.

@pulkit6559
Copy link

@pllim @jamienoss thankyou for the response, i'll try to tackle the easier ones first and then see if i could come up with something for this. :)

jamienoss added a commit to jamienoss/astropy that referenced this issue Nov 17, 2018
Resolves astropy#8107

Signed-off-by: James Noss <jnoss@stsci.edu>
@jamienoss
Copy link
Contributor Author

Sorry @pulkit6559 I had to go ahead and fix this for #8114.

jamienoss added a commit to jamienoss/astropy that referenced this issue Nov 17, 2018
Resolves astropy#8107

Signed-off-by: James Noss <jnoss@stsci.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants