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

Performance: Combining flats is slow #645

Open
pllim opened this issue Aug 21, 2018 · 2 comments
Open

Performance: Combining flats is slow #645

pllim opened this issue Aug 21, 2018 · 2 comments

Comments

@pllim
Copy link
Member

pllim commented Aug 21, 2018

This is taken from Astropy project performance survey. Is this is not the correct repo for this issue, please advise.


I have not tried this in recent astropy versions, but the early code to combine flats was FAR slower than IRAF imcombine. I had done some reading up to discover that IRAF was very proud about how it did memory management to do this efficiently. Sorry cannot be more specific or useful than that.

@MSeifert04
Copy link
Contributor

Thanks for bringing this to our attention @pllim.

There are several places that are quite slow and memory inefficient in combine and Combiner. We are currently working on a few of these issues. But it's unlikely that we'll be faster (or as fast) as imcombine. 😅

@jpmorgen
Copy link

jpmorgen commented Dec 14, 2020

It occurs to me that considerable speed might be gained by using multiprocessing: each tile could be subdivided into sub-tiles to be farmed out to their individual combiners so that those of us who have multi-core machines (pretty much everyone these days) can enjoy a considerable boost in performance. I have started to design a system that I think would work and preserve the original memory handling subdivisions by making use of queues to provide instructions to the subprocesses and mmaped files (the astropy.fits default) to avoid opening the same file multiple times while creating the sub-tiles. Opening each file multiple times to create the chunk will still, of course, be necessary.

Is there some larger design decision that discourages the use of multiprocessing at this level? Yes, it means that upstream multiprocessed callers need to have their daemon flag set to False and divide the maximum memory and number of processes passed to combiner by the number of parent processes, but with those considerations, I think it should work OK.

If there is interest in the multiprocess route, let me know.

Thanks!

jpm

@mwcraig mwcraig modified the milestones: 2.1.1, 2.2.0 Mar 15, 2021
@mwcraig mwcraig modified the milestones: 2.2.0, 2.2.1 May 25, 2021
@mwcraig mwcraig modified the milestones: 2.2.1, 2.3 Nov 21, 2021
@mwcraig mwcraig modified the milestones: 2.3, 3.0 Jan 19, 2022
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

5 participants