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

Broadcast binary ops involving strided triangular #55798

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Sep 18, 2024

Currently, we evaluate expressions like (A::UpperTriangular) + (B::UpperTriangular) using broadcasting if both A and B have strided parents, and forward the summation to the parents otherwise. This PR changes this to use broadcasting if either of the two has a strided parent. This avoids accessing the parent corresponding to the structural zero elements, as the index might not be initialized.

Fixes #55590

This isn't a general fix, as we still sum the parents if neither is strided. However, it will address common cases.

This also improves performance, as we only need to loop over one half:

julia> using LinearAlgebra

julia> U = UpperTriangular(zeros(100,100));

julia> B = Bidiagonal(zeros(100), zeros(99), :U);

julia> @btime $U + $B;
  35.530 μs (4 allocations: 78.22 KiB) # nightly
  13.441 μs (4 allocations: 78.22 KiB) # This PR

@jishnub jishnub added performance Must go faster linear algebra Linear algebra arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug labels Sep 18, 2024
@jishnub jishnub merged commit b8093de into master Sep 19, 2024
11 of 14 checks passed
@jishnub jishnub deleted the jishnub/tri_binaryops_strided branch September 19, 2024 18:01
@jishnub jishnub added the backport 1.11 Change should be backported to release-1.11 label Sep 22, 2024
@KristofferC KristofferC mentioned this pull request Sep 24, 2024
30 tasks
KristofferC added a commit that referenced this pull request Sep 25, 2024
Backported PRs:
- [x] #55773 <!-- Add compat entry for `Base.donotdelete` -->
- [x] #41244 <!-- Fix shell `cd` error when working dir has been deleted
-->
- [x] #55795 <!-- fix #52986, regression in `@doc` of macro without REPL
loaded -->
- [x] #55829 <!-- [Dates] Make test more robust against non-UTC
timezones -->
- [x] #55641 <!-- fall back to slower stat filesize if optimized
filesize fails -->
- [x] #55744 <!-- fix #45494, error in ssa conversion with complex type
decl -->
- [x] #55783 <!-- use `inferencebarrier` instead of `invokelatest` for
1-arg `@assert` -->
- [x] #55739 <!-- Add `invokelatest` barrier to `string(...)` in
`@assert` -->

Need manual backport:
- [ ] #55798 <!-- Broadcast binary ops involving strided triangular -->

Contains multiple commits, manual intervention needed:
- [ ] #55509 <!-- Fix cong implementation to be properly random and not
just cycling. -->
- [ ] #55569 <!-- Add a docs section about loading/precomp/ttfx time
tuning -->
- [ ] #55824 <!-- Replace regex package module checks with actual code
checks -->

Non-merged PRs with backport label:
- [ ] #55845 <!-- privatize annotated string API, take two -->
- [ ] #55828 <!-- Fix some corner cases of `isapprox` with unsigned
integers -->
- [ ] #55813 <!-- Check for conflicting `@ccallable` name before JIT
registration -->
- [ ] #55743 <!-- doc: heap snapshot viewing -->
- [ ] #55741 <!-- Change annotations to use a NamedTuple -->
- [ ] #55534 <!-- Set stdlib sources as read-only during installation
-->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55453 <!-- Privatise the annotations API, for StyledStrings -->
- [ ] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
@KristofferC KristofferC mentioned this pull request Sep 30, 2024
39 tasks
KristofferC added a commit that referenced this pull request Oct 1, 2024
Backported PRs:
- [x] #55849 <!-- Mmap: fix grow! for non file IOs -->
- [x] #55863 <!-- Update TaskLocalRNG docstring according to #49110 -->
- [x] #54433 <!-- Root globals in toplevel exprs -->
- [x] #55828 <!-- Fix some corner cases of `isapprox` with unsigned
integers -->
- [x] #55890 <!-- Profile: fix order of fields in heapsnapshot & improve
formatting -->
- [x] #55884 <!-- inference: add missing `TypeVar` handling for
`instanceof_tfunc` -->
- [x] #55881 <!-- Install terminfo data under /usr/share/julia -->
- [x] #55909 <!-- do not intentionally suppress errors in precompile
script from being reported or failing the result -->
- [x] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [x] #55917 <!-- fix rawbigints OOB issues -->
- [x] #55892 <!-- TOML: Avoid type-pirating `Base.TOML.Parser` -->
- [x] #55798 <!-- Broadcast binary ops involving strided triangular -->
- [x] #55919 <!-- Limit `@inbounds` to indexing in the dual-iterator
branch in `copyto_unaliased!` -->

Contains multiple commits, manual intervention needed:
- [ ] #54009 <!-- allow extensions to trigger from packages in [deps]
-->
- [ ] #55509 <!-- Fix cong implementation to be properly random and not
just cycling. -->
- [ ] #55569 <!-- Add a docs section about loading/precomp/ttfx time
tuning -->
- [ ] #55824 <!-- Replace regex package module checks with actual code
checks -->

Non-merged PRs with backport label:
- [ ] #55932 <!-- REPL: make UndefVarError aware of imported modules -->
- [ ] #55910 <!-- Prevent extensions from blocking parallel
pre-compilation -->
- [ ] #55908 <!-- add logic to prefer loading modules that are already
loaded -->
- [ ] #55886 <!-- irrationals: restrict assume effects annotations to
known types -->
- [ ] #55871 <!-- lowering: don't reverse handler order in
`(pop-handler-list ...)` -->
- [ ] #55870 <!-- fix infinite recursion in `promote_type` for
`Irrational` -->
- [ ] #55867 <!-- update `hash` doc string: `widen` not required any
more -->
- [ ] #55851 <!-- [REPL] Fix #55850 by using `safe_realpath` instead of
`abspath` in `projname` -->
- [ ] #55813 <!-- Check for conflicting `@ccallable` name before JIT
registration -->
- [ ] #55743 <!-- doc: heap snapshot viewing -->
- [ ] #55741 <!-- Change annotations to use a NamedTuple -->
- [ ] #55534 <!-- Set stdlib sources as read-only during installation
-->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
@KristofferC KristofferC mentioned this pull request Oct 7, 2024
44 tasks
@jishnub jishnub removed the backport 1.11 Change should be backported to release-1.11 label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't sum partially initialized non-isbits Hermitian or Symmetric matrix with Diagonal or SymTridiagonal
2 participants