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

cached loading code #48

Closed
le-ander opened this issue Dec 15, 2020 · 4 comments · Fixed by #38
Closed

cached loading code #48

le-ander opened this issue Dec 15, 2020 · 4 comments · Fixed by #38
Assignees

Comments

@le-ander
Copy link
Member

I cannot see any code for actually loading a cached dataset in _load_cached()
am I missing something?

@le-ander
Copy link
Member Author

also I think the funciton breaks if self.cache_path is not set. or is this handled somewehre outside?

@davidsebfischer
Copy link
Contributor

  • Loading code was missing, I added this now.
  • I am throwing explicit caching errors now if these functionalities are used and cache_path is not set. The data zoo can still be used without caching (ie if data is only ever loaded raw), but I think that cache_path should be almost always set for our puropses.

davidsebfischer added a commit that referenced this issue Dec 16, 2020
@le-ander
Copy link
Member Author

le-ander commented Dec 16, 2020

Cool, thanks!
Some thoughts on that:

This will throw an error if self.cache_path is None

sfaira/sfaira/data/base.py

Lines 149 to 153 in 8e8756b

fn_cache = os.path.join(
self.cache_path,
self._directory_formatted_doi,
self._directory_formatted_id + ".h5ad"
)

Should we really throw an error if load_raw=False and the cached file is not found? I'd rather have interpreted load_raw as "don't use the cache file if I set this to true. When setting it to false, just load the raw one anyway if you cannot find the cached file"

assert os.path.exists(fn_cache), f"did not find cache file {fn_cache}, consider caching first"

@davidsebfischer davidsebfischer linked a pull request Jan 4, 2021 that will close this issue
@davidsebfischer
Copy link
Contributor

I implemented what you suggested, @le-ander!

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 a pull request may close this issue.

2 participants