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

Improve transpose, adjoint of vector StridedView #5

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

mtfishman
Copy link
Contributor

@mtfishman mtfishman commented Dec 15, 2023

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
Copy link
Owner

Jutho commented Dec 15, 2023

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
Copy link
Contributor Author

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

@Jutho
Copy link
Owner

Jutho commented Dec 15, 2023

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
Copy link
Owner

Jutho commented Dec 15, 2023

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

@mtfishman
Copy link
Contributor Author

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

@Jutho
Copy link
Owner

Jutho commented Dec 15, 2023

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
Copy link
Contributor Author

mtfishman commented Dec 15, 2023

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 JuliaTesting/Aqua.jl#209).

Looks like the CI needs your approval to run.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jutho Jutho merged commit 517e676 into Jutho:main Dec 15, 2023
13 checks passed
@mtfishman mtfishman deleted the transpose_vector branch December 15, 2023 22:30
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