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

Fast resampling #130

Merged
merged 16 commits into from
Aug 24, 2019
Merged

Fast resampling #130

merged 16 commits into from
Aug 24, 2019

Conversation

herjy
Copy link
Collaborator

@herjy herjy commented Aug 22, 2019

This PR introduces the new resampling scheme. It uses the separability of the sinc and also does not compute and ffts the sinc since it is a pretty stupid thing to do. After coming to my senses I realised that setting to zeros all the fourier coefficients of the transform of a vector after length//2+1 does the the trick (rect function in Fourier).
I also improved the PSF matching to remove the bug I had when the high resolution PSF was larger than the low resolution.

This new scheme speeds up the process by a large amount. On a typical run like the one in the notebook (1x250x250 + 5x50x50 pixels, 6 sources), the former version had runtimes as follows:
Setup ~ 11'
render~ 7''
fit ~ 12'
Now with the new stuff:
Setup ~ 9''
render ~ 400ms
fit ~ 1'30''
With even newer psf interpolation, setup is now down to ~800ms!

@herjy herjy requested a review from fred3m August 22, 2019 18:25
@herjy
Copy link
Collaborator Author

herjy commented Aug 22, 2019

One more thing: At the moment, the accerlerated scheme works only for images that are not rotated from one another, which is the case for HST and HSC cosmos fields for instance. In cases where the angle between both grids is non zero, this version will be slightly less effective, but should still be better than computing the full resampling matrix.

@fred3m
Copy link
Collaborator

fred3m commented Aug 22, 2019

Ok, I'm reviewing the code now. I checked and I might have found the problem with the Travis build. So I pushed the commit myself to see if it fixes it.

Copy link
Collaborator

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

This is a clever algorithm! I left a few comments for clarification and consistency and I'm glad that I finally took the time to go over the code in detail to understand how this is implemented.

In addition to the comments in the code I have a few additional comments:

  • Make sure to remove the images in the doc tutorials (ie. multiresolution.ipynb), otherwise they take up an unnecessarily large amount of space.
  • I'm not sure that we're using fftshift and ifftshift properly. I know it works, but I've been digging into numpy's dft implementation and I'm still trying to figure out why sometimes we need to shift and others we don't. This doesn't hold up this ticket explicitly but is something to think about. If you understand the inconsistency I'd be happy to be informed.
  • I wonder if it might be useful to implement some sort of sinc interpolation class that converts one WCS pixel coordinates into another. Maybe it's just me, but I found the terminology used in match_patches and the methods that called it confusing. Again, this is nothing that should hold up this ticket but might be useful to create an issue and fix this later.
  • Once the multiresolution paper is finished we should make sure to reference it in the documentation, since the implementation can be a little confusing without the accompanying docs, especially the methods that you implemented to make things faster that are not so intuitive.

scarlet/observation.py Show resolved Hide resolved
scarlet/observation.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/observation.py Show resolved Hide resolved
scarlet/observation.py Show resolved Hide resolved
Copy link
Collaborator Author

@herjy herjy left a comment

Choose a reason for hiding this comment

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

I think I have adressed everything apart from the weird axis inversion, which, I think comes from a convention in the the various reshape/indice/(x,y) ordering. I'll keep working on this to get to the bottom of it.
I have spent a large amount of time today trying to get the accelerated scheme working for rotated scenes, only to realise that it would actually make things slower. It would share the setup time with a part of render and render would then have to make a HUGE matrix multiplication, which is not viable. I think in this case, I will have to adapt Peter idea of PCA factorisation. I thought I could avoid it but well, too bad.

scarlet/observation.py Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/observation.py Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/observation.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
scarlet/observation.py Outdated Show resolved Hide resolved
scarlet/resampling.py Show resolved Hide resolved
@fred3m
Copy link
Collaborator

fred3m commented Aug 24, 2019

Looks good, feel free to merge.

@herjy herjy merged commit 37245be into master Aug 24, 2019
@herjy herjy deleted the fast_resampling branch August 24, 2019 20:25
@herjy herjy mentioned this pull request Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants