Jutho / StridedViews.jl

A Julia package to represent strided views over a parent DenseArray
MIT License
6 stars 2 forks source link

Improve `transpose`, `adjoint` of vector `StridedView` #5

Closed mtfishman closed 10 months ago

mtfishman commented 10 months ago

Hi @Jutho, this makes it so that transpose and adjoint of vector StridedView remains a StridedView.

I did this by generalizing transpose and adjoint to support both matrix and vector StridedView by calling permutedims(::StridedView), and then making sure that works for vector StridedView by calling sreshape (so this PR also fixes permutedims(::StridedView) for vector StridedView as well). Open to other approaches.

Jutho commented 10 months ago

I am a bit hesitant about this. This means that transpose(v) * v no longer yields a number, which was the whole point why the Transpose objects were created in Julia Base. While the transpose behaviour is not officially specified in the AbstractArray interface, I do feel like this constitutes a violation of the AbstractArray interface that I would prefer to avoid.

mtfishman commented 10 months ago

Oh good point, I hadn't considered that usage of transpose/adjoint.

Jutho commented 10 months ago

I think adding these lines below line 172 would also resolve your issue:

function sreshape(a::LinearAlgebra.AdjointAbsVec, newsize::Dims)
    return sreshape(conj(StridedView(adjoint(a))), newsize)
end
function sreshape(a::LinearAlgebra.TransposeAbsVec, newsize::Dims)
    return sreshape(StridedView(transpose(a)), newsize)
end
Jutho commented 10 months ago

From CI, it seems like I will also have to check what is going on with Aqua.jl ?

mtfishman commented 10 months ago

That looks like a good fix, thanks. I can update this PR with that fix and look into the Aqua issue.

Jutho commented 10 months ago

That would be great, it's getting a bit late here to focus and do stuff properly. Feel free to bump the patch version while you are at it, then I can merge tomorrow morning if all lights turn green and immediately register the new version.

mtfishman commented 10 months ago

Ok, I changed it over to your fix, updated the tests, and updated to the latest Aqua syntax (I just removed the project_toml_formatting keyword argument since it was removed in https://github.com/JuliaTesting/Aqua.jl/pull/209).

Looks like the CI needs your approval to run.

codecov[bot] commented 10 months ago

Codecov Report

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

Comparison is base (00be762) 93.52% compared to head (feefe73) 93.67%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5 +/- ## ========================================== + Coverage 93.52% 93.67% +0.14% ========================================== Files 4 4 Lines 170 174 +4 ========================================== + Hits 159 163 +4 Misses 11 11 ```

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