Skip to content

Conversation

ethanbb
Copy link
Collaborator

@ethanbb ethanbb commented Sep 8, 2025

Fixes #80, fixes #338. This started as a fix to #80 and the scope expanded a bit because I also wanted to get typing right.

Here are the main changes:

  • The cache no longer separates arguments into args and kwargs; it builds a dict of all arguments regardless of how they were passed, excluding return_copy and the extensions instance, and uses that for comparisons with cache items. This prevents unnecessary cache misses when functions are called in different ways. The "args" column was removed.
  • return_copy is enforced to be a keyword-only argument. Also, the default value of return_copy is actually read and used by the cache decorator. If return_copy is passed to the decorator, that value is always used and it cannot be passed to the decorated function (this feature is currently not used)
  • The cache size calculation is fixed to include the CNMF object. It may be slightly different to the previous calculation because I changed the calculation for CNMF and lists/tuples to work recursively.
  • Changed the RAM cache drop policy to drop until the total size, including the new item, is below the limit (unless that is impossible because the new item is larger than the limit by itself)
    • I suppose this could be changed to not even store the new item if it's larger than the limit, however maybe the better policy then is to not evict other items in that case because there's no benefit to doing so
    • Edit: just added this behavior in a new commit
  • I added an "added_time" column to the cache; I am using this in tests of whether 2 cache entries are the same or not, regardless of whether they refer to the same object. (If this is flaky due to timing, we could also add a unique ID).
  • I added a "bytes" column to the cache so each item's size only has to be computed once.
  • Cleaned up the cache wrapper function to avoid repetition
  • Fixed a bug where the cache dataframe was copied rather than modified inplace when items were evicted (at least this was a bug for the test)
  • Made _component_indices_parser more precise wrt types...I was going to do something more drastic but realized it's better not to change the signature of functions with decorators.
  • Added wrapsmethod to use instead of functools.wraps because the latter doesn't work well with methods when type checking, for some reason. This way at least pylance correctly recognizes the signatures of decorated methods in cnmf.py and highlights some minor errors regarding them. I didn't fix most of these though because they're really from type ambiguity in CaImAn and I didn't want to add a bunch of assertions to band-aid that.
  • Fixed get_temporal only adding either background or residuals, but not both even if both were requested (this seemed like a bug)
  • Changed test_cache to take these changes into account.

@ethanbb ethanbb requested a review from kushalkolar September 8, 2025 22:22
elif add_residuals:
temporal += cnmf_obj.estimates.f

if add_residuals:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change in behavior here but it seemed like a bug...if background + residuals is not allowed it should at least warn or error.

@ethanbb
Copy link
Collaborator Author

ethanbb commented Sep 8, 2025

Oh shoot this doesn't work in 3.9. Are we still supporting 3.9?

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.

Cache size calculation does not take size of CNMF object into account return_copy should probably be dropped from kwargs when checking cache hits
1 participant