Repository navigation
Fix convolve_fft_parameters psf_pad and fft_pad, which were being ignored - #4366
Conversation
There was a problem hiding this comment.
To help prevent unnecessary looping over the whole mask arrays, I'd write it as
if (not interpolate_nan and not quite and
(np.any(nanmasarray) or np.any(nanmaskkernel))):
(python will short-circuit the if-statement; np.any seems more logical than counting).
|
I would consider this a bug fix... |
There was a problem hiding this comment.
Replace single backticks with double backticks.
|
Actually, above comments aside, could you remove the profiling code from this PR? It seems pretty separate. Would be nice to have in a separate change though. |
There was a problem hiding this comment.
The argument handling updates all make sense--I think they should just be committed as a bug fix.
|
@embray sure, can split this into two. I also agree about the plot instead of table. It probably doesn't belong in the code, that was just quickest. I'll at least separate these tomorrow, but I don't know if I'll get to cleaning up the profiler. |
|
No hurry on the profiler obviously, but thanks for adding it nonetheless. |
doing nothing before. As it is, psf_pad and fft_pad have 100% overlap with boundary.
but this is crucial for preventing my computer from crashing!)
d0262ad to
7dbb848
Compare
|
@keflavich - can you clarify if this if you consider this an API change? My thinking is that this means that in some cases code that ran before now will instead raise an error. That's probably good going forward, but is still an API change/new feature (and hence belongs in the next major release). Do you agree? |
|
@eteq I would still consider this a bugfix rather than an API change. The API change was supposed to happen ~ a year ago, but I missed some of the possible conditions so that it was never actually implemented. The documented API hasn't changed, at least, except to clarify when and whether |
|
@embray @astrofrog - so what do we want to do about this given @keflavich's statement above? sneak it in for 1.1, or schedule it for 1.1.1? And should it also go into 1.0.x? (My thinking is yes at least to the latter, unsure for the former question) |
|
(Oh, and this is missing a changelog entry, @keflavich - feel free to put it in whichever version you like and then we can always reorganize later when we decide where it should finally go) |
|
After thinking a bit (and discussing w/ @astrofrog), it's probably better to have this be in 1.1.1 but not 1.1.0, unless there's some critical fix that ends up requiring a 1.1rc3. |
|
Right, let's delay this for now, I think. |
|
Alright, I'm going to go ahead and make the command decision to merge this and backport it to 1.0.9. I'll trust @keflavich's judgement that it's actually a "true" bug. |
Fix convolve_fft_parameters psf_pad and fft_pad, which were being ignored
Fix convolve_fft_parameters psf_pad and fft_pad, which were being ignored
Fix convolve_fft_parameters psf_pad and fft_pad, which were being ignored
The parameters
psf_padandfft_padwere being ignored ifboundary!=None,which was intentional but was opaque to users. Now, errors will be raised if
incompatible parameters are passed and the keywords will be respected when
specified.
psf_pad,fft_pad, andboundaryare somewhat redundant, but there is goodreason to include both:
boundarycontrols something "physical", i.e., it determineswhat the edge effects will be.
psf_padis closely related to this, and perhapsis genuinely redundant. However, both
psf_padandfft_padcan seriously affectthe performance of the convolution in non-obvious ways. Therefore, I think they
all deserve to be included.
I have also included as a string a report on the performance of
convolve_fftunder different conditions. This came up because a user was complaining they
were waiting forever for a convolution of a 6000x6000 with a 3000x3000 array,
which with the default parameters will create a set of 16384x16384 arrays!