search

willow-ahrens / Finch.jl

Sparse tensors in Julia and more! Datastructure-driven array programing language.
http://willowahrens.io/Finch.jl/
MIT License
158 stars 15 forks source link

Swizzle back in `copyto!` #390

Closed mtsokol closed 7 months ago

mtsokol commented 7 months ago

Hi @willow-ahrens,

I took a look at #387 and I think that return ret.body causes omitting "swizzling" back to the desired dimensionality in some copyto! functions. I just started learning how it works so I might have misunderstood it.

fixes #387

willow-ahrens commented 7 months ago

Awesome, thanks so much for the fix! Could you add a quick test case and then I'll merge?

codecov[bot] commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (2f50d2d) 76.44% compared to head (f54de8d) 76.40%.

:exclamation: Current head f54de8d differs from pull request most recent head 7ecbf97. Consider uploading reports for the commit 7ecbf97 to get more accurate results

Files Patch % Lines
src/interface/copy.jl 25.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #390 +/- ## ========================================== - Coverage 76.44% 76.40% -0.05% ========================================== Files 83 83 Lines 7299 7298 -1 ========================================== - Hits 5580 5576 -4 - Misses 1719 1722 +3 ```

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

mtsokol commented 7 months ago

@willow-ahrens I added a test that checks permutedims for 3d tensor with different shapes. I think that the current fix isn't sufficient as some of the test cases fail with incorrect shapes compared to Base.permutedims.

willow-ahrens commented 7 months ago

I think the main issue here is that Finch seems to drop the swizzle wrapper on the output. The fix will be a little tricky, I'll need to look at it tonight and get back to you.

mtsokol commented 7 months ago

Thank you! For me it's a bit difficult to follow the code, considering that tocopy! both returns a value and mutates passed argument for the same purpose. And it seems that it matters which one is used in different places.

willow-ahrens commented 7 months ago

Yes, I think a big mistake in finch design was immutable dimensions. We should fix that, feel free to file an issue

willow-ahrens commented 7 months ago

The main issue is here: https://github.com/willow-ahrens/Finch.jl/blob/0a536c7f43ef76ffd55ca0a927e654466e1efda8/src/transforms/wrapperize.jl#L181-L188

In these lines of code, Finch is stripping the swizzle off of the tensor, and doesn't have a way to re-wrap the tensor before return.