finch-tensor / finch-tensor-python

Sparse and Structured Tensor Programming in Python
MIT License
8 stars 3 forks source link

Use `jl.permutedims` in `permute_dims` #44

Closed mtsokol closed 5 months ago

mtsokol commented 5 months ago

Hi @hameerabbasi @willow-ahrens,

This small fix makes permute_dims use jl.permutedims instead of jl.swizzle.

It's needed to make transpositions work in the lazy mode, because for swizzle(lazy(tensor), ...) we get:

ERROR: MethodError: swizzle(::Finch.LazyTensor{Float64, 3}, ::Int64, ::Int64, ::Int64) is ambiguous.

Candidates:
  swizzle(body, dims::Int64...)
    @ Finch ~/JuliaProjects/Finch.jl/src/tensors/combinators/swizzle.jl:80
  swizzle(arr::Finch.LazyTensor, dims...)
    @ Finch ~/JuliaProjects/Finch.jl/src/interface/lazy.jl:63

and permutedims doesn't raise it.

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (main@0c3a202). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #44 +/- ## ======================================= Coverage ? 91.37% ======================================= Files ? 7 Lines ? 649 Branches ? 0 ======================================= Hits ? 593 Misses ? 56 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

willow-ahrens commented 5 months ago

I think we should fix that ambiguity error too though. I'll add the issue to finch.jl

mtsokol commented 5 months ago

Sure! For now it's a small fix here in finch-tensor.

mtsokol commented 5 months ago

Can we add a regression test? Something which ensures permute_dims will always compile?

Sure! Done! I extended test_permute_dims so it also runs lazy mode (it passes here and fails in a main branch).

hameerabbasi commented 5 months ago

Thanks for the diagnosis and the work put towards fixing this, @mtsokol!