Skip to content

Reduce peak memory use by not reading full file to get RA DEC columns#144

Merged
jeanconn merged 1 commit intomasterfrom
low-mem
Nov 9, 2022
Merged

Reduce peak memory use by not reading full file to get RA DEC columns#144
jeanconn merged 1 commit intomasterfrom
low-mem

Conversation

@jeanconn
Copy link
Copy Markdown
Contributor

@jeanconn jeanconn commented Nov 8, 2022

Description

Reduce peak memory use by reading just the RA and DEC fields to get them (used for indexing into the agasc).

Interface impacts

Testing

Unit tests

  • Mac

Independent check of unit tests by @taldcroft:

  • Mac

Functional tests

I looked at this before and after with memory profiler output and confirmed that the change means that, using the proseco agasc file as an example, only the 60Mb columns are read into memory instead of the file (which peaks more at ~600Mb).

@jeanconn
Copy link
Copy Markdown
Contributor Author

jeanconn commented Nov 8, 2022

I considered updating the code to remove the explicit cache, and provide an option to not-cache the RA DECs altogether... but maybe it makes sense to limit scope creep.

Copy link
Copy Markdown
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good!

@taldcroft
Copy link
Copy Markdown
Member

And even though I'm now fully convinced that returning from within a context block will run the __exit__ method (https://stackoverflow.com/questions/9885217/in-python-if-i-return-inside-a-with-block-will-the-file-still-close), I still think it smells bad. (i.e. it has a popular a stack overflow answer so it is confusing).

@jeanconn jeanconn merged commit 4f8b167 into master Nov 9, 2022
@jeanconn jeanconn deleted the low-mem branch November 9, 2022 04:39
@javierggt javierggt mentioned this pull request May 17, 2023
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.

2 participants