AllenInstitute / visual_behavior_analysis

Python package for analyzing behavioral data for Brain Observatory: Visual Behavior
Other
21 stars 6 forks source link

event file gets loaded way too much #695

Closed alexpiet closed 3 years ago

alexpiet commented 3 years ago

In the file visual_behavior/data_access/loading.py the events() function doesn't cache the events dataframe, it loads it from file every time the function is called. Further, when it loads it creates the events dataframe, it actually loads the events file twice.

dougollerenshaw commented 3 years ago

Might the fix be as simple as changing the @property decorator into @cached_property?

I'll investigate.

dougollerenshaw commented 3 years ago

Oh, the @cached_property decorator is only available starting in python 3.8. The SDK explicitly does not support 3.8, so we can't use it yet.

https://docs.python.org/3.8/library/functools.html

dougollerenshaw commented 3 years ago

I followed the pattern in ophys/response_analysis/response_analysis.py to apply the LazyLoadable method to events. See commit 2591f10be889845a45fc7c8c81b8870075c04aea

Here's a screenshot of a notebook showing that the second time events is loaded, the load time is now under 1 ms:

image

dougollerenshaw commented 3 years ago

@alexpiet, does this seem like a satisfactory solution?

Whenever we're able to upgrade to Python 3.8 (dependent on when the AllenSDK begins supporting it, see https://github.com/AllenInstitute/AllenSDK/issues/1652, https://github.com/AllenInstitute/AllenSDK/issues/1643, https://github.com/AllenInstitute/AllenSDK/issues/1610, https://github.com/AllenInstitute/AllenSDK/pull/1611), we should replace these LazyLoadable calls, and probably many of the @property decorators, with the newly supported @cached_property decorator.

alexpiet commented 3 years ago

That seems great!

In reality my only concern was I didnt like that it printed a message "getting LO events now" twice. I would have been happy removing the print statement. :D

dougollerenshaw commented 3 years ago

Great! Closing this issue. Note that this change is part of the epic dev branch PR #693.