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

Use lightkurve's cache instead of maintaining our own #230

Closed
wants to merge 1 commit into from
Closed

Use lightkurve's cache instead of maintaining our own #230

wants to merge 1 commit into from

Conversation

warrickball
Copy link
Contributor

Instead of trying to maintain our own caching system, why not just rely on lightkurve? This simplifies query_lightkurve to just go ahead and use lightkurve to retrieve the data, rather than going through launch_query and lookup_cached_files to see if its in the cache manually. If we want to be aggressive with this PR, launch_query and lookup_cached_files can be removed as they aren't called anywhere else (I don't think).

The downside with lightkurve's caching is that, as far as I can tell, there's no simple option for bypassing the cache and forcing data to be downloaded again. But it can be done manually (by deleting the relevant cached files) and someone can open an issue against lightkurve or open a PR to implement it if they like.

I tried to preserve the ValueError that was produced if no data is found. Happy to tweak this if you send me some examples I should check.

@warrickball
Copy link
Contributor Author

It turns out that lightkurve's caching goes through astroquery, with the side-effect that even if you've downloaded the data, it won't work if you're offline. There's already a lightkurve issue on this, which has led to an issue and two related pull requests in astroquery.

So we shouldn't merge this until those PRs are resolved and, hopefully, astroquery won't require us to be online to retrieve cached data. For now, @nielsenmb's existing code is a good enough workaround.

@grd349
Copy link
Owner

grd349 commented Jul 1, 2020

@nielsenmb @warrickball Any news on this from the lightkurve/astropy side? Will this PR likely just become the specification of the latest version of lk or astropy?

@nielsenmb
Copy link
Collaborator

nielsenmb commented Jul 2, 2020 via email

@warrickball
Copy link
Contributor Author

I just had a look at the PRs and issues I linked before and it looks like there hasn't been any activity. Unless we want to poke, we provably just want to stick to our own caching system.

I opened this issue before I was aware that lightkurve/astroquery have their own caching issues. You're welcome to close this while we wait to see if they get patched, or leave this open to keep reminding us to do so.

@grd349
Copy link
Owner

grd349 commented Jul 2, 2020

@nielsenmb I guess you want to leave this open as you say you are working on something?

@nielsenmb
Copy link
Collaborator

nielsenmb commented Jul 2, 2020 via email

@nielsenmb
Copy link
Collaborator

LK's lookup is much faster now so no need for this anymore I think.

@nielsenmb nielsenmb closed this Sep 11, 2024
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.

3 participants