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

getproperty for Adjoint/Transpose wrappers #97

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented May 31, 2023

This is the first step towards making adjoint/transpose return wrappers, and having optimized factorization defined for these wrappers. In this PR, only getproperty is defined for Adjoint/Transpose wrappers.

@andreasnoack
Copy link
Member

Are we sure it's beneficial to return wrapped types instead of materializing them? The materialized versions should be quite cheap for many of the matrices here, no?

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (52775d6) 95.08% compared to head (1a56721) 95.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   95.08%   95.15%   +0.06%     
==========================================
  Files           8        8              
  Lines         896      908      +12     
==========================================
+ Hits          852      864      +12     
  Misses         44       44              
Impacted Files Coverage Δ
src/ToeplitzMatrices.jl 91.66% <100.00%> (+4.16%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jishnub
Copy link
Member Author

jishnub commented May 31, 2023

It should be cheap, but since the docstrings of adjoint and transpose suggest that these are lazy wrappers, and that the adjoint of an adjoint unwraps the parent, perhaps it's best to respect these properties. If possible, it's better to not materialize the matrices.

@andreasnoack
Copy link
Member

I think the docstring is inaccurate. E.g. I believe adjoint(::Hermitian) is still a noop.

@jishnub
Copy link
Member Author

jishnub commented May 31, 2023

I see, although adjoint(adjoint(H::Hermitian)) === H still holds. In any case, reducing memory allocation is perhaps a good idea? This might make it easier to port such matrices to GPUs and other memory-limited systems

@andreasnoack
Copy link
Member

Reducing allocations is generally a good thing but my question is how much is reduced there. For a general toeplitz, you'd just swap the two vectors (e.g. in the real case), no?

@jishnub
Copy link
Member Author

jishnub commented Jun 1, 2023

In the real Toeplitz case, this may not matter much, but there are other cases where this will save allocating one or two vectors

julia> C = Circulant(fill(2,1000));

julia> @btime adjoint($C);
  1.704 μs (2 allocations: 15.88 KiB)

julia> T = Toeplitz(fill(2im, 1000), fill(2im, 1000));

julia> @btime adjoint($T);
  2.658 μs (2 allocations: 31.50 KiB)

I'm only suggesting using a wrapper in cases where it would have allocated otherwise. Unfortunately, this would mean departing from the type hierarchy, but hopefully this may be mitigated by defining appropriate methods for the wrappers. I plan to add a benchmark script to track regressions. This is anyway a longer-term vision, and this PR should largely be harmless.

@andreasnoack
Copy link
Member

Sure. Wrapped types just add a lot of complexity and also potential compilation overhead so it's just good to avoid them when it free or cheap enough.

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.

None yet

2 participants