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

Fix issues with LazyBufferCache #21

Merged
merged 3 commits into from
May 6, 2022
Merged

Conversation

danielwe
Copy link
Contributor

@danielwe danielwe commented May 6, 2022

This PR fixes some bugs and aligns code and documentation for LazyBufferCache. In order of commits:

  • b = LazyBufferCache(f) produces caches of arbitrary lengths when f is a closure.
    • MWE:
      using PreallocationTools
      
      LBC(n) = LazyBufferCache(_ -> n)
      
      b = LBC(5)
      b[ones(1)]  # Arbitrary length or OOM, should be length 5
    • Cause: f not being passed through to new in the constructor, making b.lengthmap an uninitialized instance of typeof(f). (I'm surprised this is even allowed.)
    • Solution: Pass f through to new
  • b[x] and b[y] return the same buffer as long as x and y have the same eltype, regardless of sizes and array types/storage
    • MWE:
      using CUDA
      using PreallocationTools
      
      b = LazyBufferCache()
      x = ones(4)
      b[x]  # Length 4 as expected
      b[x[1:3]]  # Still length 4, should be length 3
      b[CuArray(x[1:3])]  # Still trying to return a CPU array of length 4 => typeassert failure in getindex; should be CuArray of length 3
    • Cause: only eltype(x) used as key to b.bufs, not size and array type
    • Solution: use (typeof(x), f(size(x))) as key
    • Outstanding issue: the typeassert that ensures output type inferability relies on typeof(similar(u, size)) === typeof(u). This does not hold for all AbstractArrays; ranges like 1:5 are the canonical counterexamples. This hardly seems relevant for LazyBufferCache use cases, so I've left it as is.
  • Docs and behavior inconsistent: f is actually used as size map, not length map, so the LBC can both accept and return arrays of arbitrary dimension, not just vectors. In particular, matrices are already supported, contrary to what a comment in the code claims.
    • Solution: Update docs and variable names, e.g., from b.lengthmap to b.sizemap.

Some tests are probably in order, I'll get to that tomorrow.

danielwe added 3 commits May 5, 2022 23:47
The type of `f` is not enough, the value is also needed if it's
a closure
Required to behave as promised in the docstring
The length map has been a size map all along
@ChrisRackauckas
Copy link
Member

Awesome, these changes look great, thanks!

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