Repository navigation
SPHEREx cleanup: refactor cutouts/psf, preserve MEF headers, drop redundant SDT README #396
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5fb33b0
Use astroquery/SkyCoord, overwrite=False, quantity-aware plots, prese…
jkrick 00c9664
Fold directory contents into SDT notebook, remove redundant README
jkrick 556f68c
Open spherex_intro.md's spectral image with a context manager (#129)
jkrick decd5ae
Drop removed SDT README from the Fornax deploy manifest
jkrick bfe4f43
Treat RemoteDisconnected as a transient read error in cutouts/psf
jkrick bda8a54
Keep only per-cutout version; leave MEF primary header blank
jkrick File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be saved to a variable with
.copy()to be robust. I recall doing something similar and finding that the code would sometimes error because the data disappeared (I suspect that because the file is closed the next garbage collection removes it from memory). But if I'm wrong I'd love to see some documentation (if any exists) because this comes up for me occasionally.(and then use
hdu_datain place ofhdu.databelow)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and I think the current code is OK as is. With
memmap=False, touchinghdu.datareads the array into ordinary memory and stores it on the HDU.hdulistkeeps a reference to every HDU, so garbage collection can't free the data after the file closes. To test it, I built a 5-extension file (float image, int image, scaled image, and a table like WCS-WAVE). I opened it this way, let thewithblock close it, forcedgc.collect()while allocating other memory, and compared. Every extension was still intact, and none of the arrays were backed by a memory map or file buffer.The failure I could reproduce is lazy loading: an HDU that isn't touched before the file closes raises "file was closed before all HDUs were read" afterwards. That's why the loop touches every
.data. The usual.copy()advice is for the defaultmemmap=True(astropy FAQ, "Too many open files").Docs: https://docs.astropy.org/en/stable/io/fits/index.html says that after closing, "the headers and data may or may not be accessible depending on whether the data are touched and if they are memory-mapped". The FAQ (https://docs.astropy.org/en/stable/io/fits/appendix/faq.html) says
memmap=False"will force data to be read entirely into memory."Also, the later cells use
hdulist[...]directly, includingWCS(..., fobj=hdulist), so onehdu_datavariable in the loop would keep only the last extension's data.