Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1651 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 67 67
Lines 22688 22705 +17
=======================================
+ Hits 22674 22691 +17
Misses 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This makes a few memory handling improvements to the uvfits reader (and maybe the MWA correlator FITS reader).
It turns out that when using
astropy.io.fits.openwithmemmap=True, the python garbage collector does not release the memory promptly upon exiting the context manager. We were always usingmemmap=True, but it's really only needed when doing partial reads. This PR changes the calls to theastropy.io.fits.opento only usememmap=Truewhen doing partial reads. This leads to significant memory improvements for the uvfits reader when reading the whole file. It's not clear if there are any improvements to the MWA correlator FITS reader in terms of memory usage, but it seemed better to be consistent.I also refactored the uvfits reader to remove a temporary array that was being used to store and manipulate the data stored in the primary uvfits HDU before assigning parts of it to the data_array, nsample_array and flag_array. I now have it just assign the full data from the HDU to the data_array and do all the manipulations in place in that array. It feels a bit yucky but leads to much better memory usage.
Below are plots of the memory usage as measured by
memrayfor a small script that just reads in an MWA uvfits file (the blue resident size is most important). There is some run-to-run variability to these plots that I don't fully understand, so I'm including 2 plots for each codebase, from different days but all three codebase runs were run back to back on the 2 different days (all on my laptop):main branch:


After using


memmap=False:After refactoring out the temporary array:


Note that the peak memory use doesn't change much (and is seems quite variable in the last set of plots), but there's a consistent big change in the final memory usage at the end of the script. If you are doing anything with the object after reading it, that becomes your baseline memory usage for the next step. I actually stumbled on this when trying to profile something else (frequency averaging) and discovered that the memory usage was much higher when I used a script that started by reading in a uvfits file vs when I started by reading in the same data saved as a uvh5 file.
Similar plots for the MWA Correlator FITS reader seem to be dominated by run-to-run variability -- I don't see a consistent improvement with these changes, but I don't see it getting materially worse either.
We could certainly make similar changes in the calfits and beamfits readers, but I wasn't sure I should cram them into this PR.
Motivation and Context
Types of changes
Checklist:
Other: