Solve all problems with extra indirection

[timholy]

The getindex and setindex! methods for Trues are limited
while also risking ambiguities. This replaces those definitions
with a specialization for to_indices that avoids such problems.

Closes #162

[codecov]

Codecov Report

Merging #163 (44e223b) into master (a8bffd3) will increase coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   96.25%   96.49%   +0.24%     
==========================================
  Files           4        4              
  Lines         640      627      -13     
==========================================
- Hits          616      605      -11     
+ Misses         24       22       -2     

Impacted Files	Coverage Δ
src/trues.jl	100.00% <100.00%> (+13.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8bffd3...44e223b. Read the comment docs.

[ChrisRackauckas]

Boom. Thanks. I knew you'd be the one to know the solution haha

[timholy]

The best answer to ambiguities is almost always to delete methods. After all, if you have 0 methods, it's guaranteed there will be 0 ambiguities 😆.

[dlfivefifty]

Note the to_indices call is not actually invoked...

I think this is probably more proof that the tests are not testing the added functionality of the overload, rather that it doesn't break. But TBH I don't really understand the use case for these special getindex and setindex! overloads so I'm fine with deleting them (along with the to_indices overload you added).... The original author @JeffFessler can of course propose a better alternative with relevant tests.

[timholy]

Here's a test that now passes that didn't before:

    mask = OffsetArray(Trues(dim), 0:3, -2:2)
    x = OffsetArray(randn(dim), axes(mask)...)
    @test x[mask] == vec(x)
    @test_throws BoundsError x[parent(mask)]

Unfortunately, simply loading OffsetArrays introduces an ambiguity with your reshape methods. If you delete all your support for reshape that goes away, and the only thing that fails are your ≡ Fill(2, 2, 3)-type tests. There may be good ways to solve that too, but I haven't looked. The first question is: how important is it to avoid the ReshapedArray wrapper?

[timholy]

Note the to_indices call is not actually invoked...

This should fix it.

TBH I don't really understand the use case for these special getindex and setindex! overloads

I am guessing it's a performance optimization to save time in checking each boolean value.

[dlfivefifty]

Removing reshape is a step too far, I think. Feels like a race to the bottom where we in the end remove all functionality.... and the argument can be made that OffsetArrays.jl should remove it's reshape overloads.

In the end ambiguities are just a fact of life in combining Julia packages, trying to avoid them completely is not realistic. This is where a glue package called OffsetFillArrays.jl or whatever would come in (with future Pkg support for making it mandatory when one uses the two packages together)

[dlfivefifty]

I am guessing it's a performance optimization to save time in checking each boolean value.

Yes of course, the question is when would anyone in practice call x[Trues(...)] in the first flace

[timholy]

I bet there's a way around it. You're right, though, that Base has reshape methods that specialize both the array-type and the shape-types. That's usually a sign of poor design (fault: mine). See https://docs.julialang.org/en/v1/manual/methods/#man-method-design-ambiguities

[dlfivefifty]

I wonder if long term it’s better to make an ordering of which comes first, array overloads or shapes. If so we could do

reshape(A…) = reshapebyshapes(A…)
reshapebyshapes(A…) = reshapebyarraytype(A…)
reshapebyarraytype(A…) = defaultreshape(A…)

Then if one only does overloads by

reshapebyshapes(A, ::MyShape) = …
reshapebyarraytype(A::MyArray, sh…) = …

We would avoid ambiguity

But would need a change in Base

[timholy]

Yep, it's a tricky design problem. No time to look into it now, but it turns out that when I tested reshape method deletion I also deleted all the support routines. If you keep those and just delete the reshape methods, everything passes. Moreover, this reduces the number of ambiguities in this package (even without loading OffsetArrays) from 56 to 52 (Julia 1.8).

Shall I pile that on here or do you want a separate PR?

[dlfivefifty]

Does it still return a Fill ? I’m pretty sure I rely on this downstream (in particular permutedims which I believe calls reshape )

[timholy]

Yes (the tests check as much)

[JeffFessler]

Yes of course, the question is when would anyone in practice call x[Trues(...)] in the first flace

My use case for x[Trues(...)] is in ImageGeoms.jl and in fact the Literate docs for that package specifically refers to this specific feature of FillArrays: https://juliaimagerecon.github.io/ImageGeoms.jl/stable/examples/2-mask

So I very much hope that some version of x[Trues(...)] remains supported in the most efficient way possible. The original version basically mapped x[Trues(...)] into vec(x) which (as you know) is non-allocating (akin to a view). It looks to me like the proposed version will end up doing A[vec(LinearIndices(A))] which will be allocating. This would significantly degrade performance if I am understanding correctly.

Is there some other way to restrict the types of this getindex method to avoid method ambiguities? I would like to retain the memory efficiency of vec(x) somehow.

I need to think more about whether the proposed deletion of setindex! will degrade performance. Can you give me a couple days to run some timing tests?

On a perhaps slightly related note, to support all the functionality needed efficiently I also had to write a getindex! method that (to my surprise) was not supported directly in Base:
https://github.com/JuliaImageRecon/ImageGeoms.jl/blob/ac86239b72518c6b920439d49f74fcf728707ab2/src/mask.jl#L17
That code does use LinearIndices but it is inherently non-allocating.

[dlfivefifty]

@JeffFessler Yes no rush. It's possible to fix the issue without deleting everything.

Can you add tests so that we make sure we don't lose the functionality you need?

[timholy]

Yes, that's a good idea. I really doubt there will be any performance implications but you never know unless you check. No rush here.

[ChrisRackauckas]

Can we merge this? I think that's been ample time to test.

[ChrisRackauckas]

and a tag?

[dlfivefifty]

Sure, done. Note (I assume) you have admin rights and can just tag a new release yourself in the future

[dlfivefifty]

Err no one bumped the version. If you want a PR tagged please up the version first.