-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: master
Are you sure you want to change the base?
Conversation
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 ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
It should be cheap, but since the docstrings of |
I think the docstring is inaccurate. E.g. I believe |
I see, although |
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? |
In the real 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. |
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. |
This is the first step towards making
adjoint
/transpose
return wrappers, and having optimized factorization defined for these wrappers. In this PR, onlygetproperty
is defined forAdjoint
/Transpose
wrappers.