Skip to content

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Apr 12, 2024

This makes the following type-stable:

julia> B = Bidiagonal(rand(3), rand(2), :U);

julia> @inferred (B -> B .* 2)(B)
3×3 Bidiagonal{Float64, Vector{Float64}}:
 0.3929  1.93165    
        1.61301  1.00202
                1.96483

Similarly, for other operations involving a single Bidiagonal and numbers. This is not type-stable on master, as the number of Bidiagonal matrices in a broadcast operation is not tracked (even though this is used in promoting the uplo). Since the uplo can't be constant-propagated, we count this by introducing an additional flag in the promotion mechanism, which is entirely determined by the types of the terms in the broadcast operation.

@jishnub jishnub added linear algebra Linear algebra arrays [a, r, r, a, y, s] labels Apr 12, 2024
@jishnub jishnub requested a review from N5N3 April 19, 2024 03:20
@N5N3
Copy link
Member

N5N3 commented Apr 20, 2024

The motivation here makes sense to me.

I'm not sure if it's a good idea to track this info during find_uplo. It seems clearer to add another help function.
And once we want to fix JuliaLang/LinearAlgebra.jl#1063, a general version of this check looks useful to keep basic stability.

N5N3 added a commit to N5N3/julia that referenced this pull request Apr 22, 2024
1. size-1 StructuredMatrix should behave like scalar during broadcast. Thus their `fzero` should return the only element.
(fix #54087)

2. But for simple broadcast with only one StructuredMatrix, we can keep stability as the structure is "preserved" even for size-1 case. Thus `count_structedmatrix` is added to check that.

3. `count_structedmatrix` is fused to keep `Bidiagonal` stability.
(replace JuliaLang#54067)
@jishnub
Copy link
Member Author

jishnub commented May 1, 2024

I've borrowed the idea from #54190 so that this PR may be assessed independently while we await a decision on the other. The changes here should be straightforward, and the other PR may build on this.

@N5N3 N5N3 added the merge me PR is reviewed. Merge when all tests are passing label May 2, 2024
@jishnub jishnub added the backport 1.11 Change should be backported to release-1.11 label May 2, 2024
@jishnub jishnub merged commit 685f527 into master May 2, 2024
@jishnub jishnub deleted the jishnub/bidiagbroadcast branch May 2, 2024 08:33
@jishnub jishnub removed the merge me PR is reviewed. Merge when all tests are passing label May 2, 2024
@KristofferC KristofferC mentioned this pull request May 6, 2024
59 tasks
KristofferC pushed a commit that referenced this pull request May 6, 2024
…#54067)

This makes the following type-stable:
```julia
julia> B = Bidiagonal(rand(3), rand(2), :U);

julia> @inferred (B -> B .* 2)(B)
3×3 Bidiagonal{Float64, Vector{Float64}}:
 0.3929  1.93165   ⋅
  ⋅      1.61301  1.00202
  ⋅       ⋅       1.96483
```
Similarly, for other operations involving a single `Bidiagonal` and
numbers. This is not type-stable on master, as the number of
`Bidiagonal` matrices in a broadcast operation is not tracked (even
though this is used in promoting the `uplo`). Since the `uplo` can't be
constant-propagated, we count this by introducing an additional flag in
the promotion mechanism, which is entirely determined by the types of
the terms in the broadcast operation.

---------

Co-authored-by: N5N3 <[email protected]>
(cherry picked from commit 685f527)
KristofferC added a commit that referenced this pull request May 28, 2024
Backported PRs:
- [x] #53665 <!-- use afoldl instead of tail recursion for tuples -->
- [x] #53976 <!-- LinearAlgebra: LazyString in interpolated error
messages -->
- [x] #54005 <!-- make `view(::Memory, ::Colon)` produce a Vector -->
- [x] #54010 <!-- Overload `Base.literal_pow` for `AbstractQ` -->
- [x] #54069 <!-- Allow PrecompileTools to see MI's inferred by foreign
abstract interpreters -->
- [x] #53750 <!-- inference correctness: fields and globals can revert
to undef -->
- [x] #53984 <!-- Profile: fix heap snapshot is valid char check -->
- [x] #54102 <!-- Explicitly compute stride in unaliascopy for SubArray
-->
- [x] #54070 <!-- Fix integer overflow in `skip(s::IOBuffer,
typemax(Int64))` -->
- [x] #54013 <!-- Support case-changes to Annotated{String,Char}s -->
- [x] #53941 <!-- Fix writing of AnnotatedChars to AnnotatedIOBuffer -->
- [x] #54137 <!-- Fix typo in docs for `partialsortperm` -->
- [x] #54129 <!-- use correct size when creating output data from an
IOBuffer -->
- [x] #54153 <!-- Fixup IdSet docstring -->
- [x] #54143 <!-- Fix `make install` from tarballs -->
- [x] #54151 <!-- LinearAlgebra: Correct zero element in
`_generic_matvecmul!` for block adj/trans -->
- [x] #54213 <!-- Add `public` statement to `Base.GC` -->
- [x] #54222 <!-- Utilize correct tbaa when emitting stores of unions.
-->
- [x] #54233 <!-- set MAX_OS_WRITE on unix -->
- [x] #54255 <!-- fix `_checked_mul_dims` in the presence of 0s and
overflow. -->
- [x] #54259 <!-- Fix typo in `readuntil` -->
- [x] #54251 <!-- fix typo in gc_mark_memory8 when chunking a large
array -->
- [x] #54276 <!-- Fix solve for complex `Hermitian` with non-vanishing
imaginary part on diagonal -->
- [x] #54248 <!-- ensure package callbacks are invoked when no valid
precompile file exists for an "auto loaded" stdlib -->
- [x] #54308 <!-- Implement eval-able AnnotatedString 2-arg show -->
- [x] #54302 <!-- Specialised substring equality for annotated strs -->
- [x] #54243 <!-- prevent `package_callbacks` to run multiple time for a
single package -->
- [x] #54350 <!-- add a precompile signature to Artifacts code that is
used by JLLs -->
- [x] #54331 <!-- correctly track freed bytes in
jl_genericmemory_to_string -->
- [x] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [x] #54335 <!-- When accessing the data pointer for an array, first
decay it to a Derived Pointer -->
- [x] #54239 <!-- Make sure `fieldcount` constant-folds for `Tuple{...}`
-->
- [x] #54288
- [x] #54067
- [x] #53715 <!-- Add read/write specialisation for IOContext{AnnIO} -->
- [x] #54289 <!-- Rework annotation ordering/optimisations -->
- [x] #53815 <!-- create phantom task for GC threads -->
- [x] #54130 <!-- inference: handle `LimitedAccuracy` in
`handle_global_assignment!` -->
- [x] #54428 <!-- Move ConsoleLogging.jl into Base -->
- [x] #54332 <!-- Revert "add unsetindex support to more copyto methods
(#51760)" -->
- [x] #53826 <!-- Make all command-line options documented in all
related files -->
- [x] #54465 <!-- typeintersect: conservative typevar subtitution during
`finish_unionall` -->
- [x] #54514 <!-- typeintersect: followup cleanup for the nothrow path
of type instantiation -->
- [x] #54499 <!-- make `@doc x` work without REPL loaded -->
- [x] #54210 <!-- attach finalizer in `mmap` to the correct object -->
- [x] #54359 <!-- Pkg REPL: cache `pkg_mode` lookup -->

Non-merged PRs with backport label:
- [ ] #54471 <!-- Actually setup jit targets when compiling
packageimages instead of targeting only one -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #54323 <!-- inference: fix too conservative effects for recursive
cycles -->
- [ ] #54322 <!-- effects: add new `@consistent_overlay` macro -->
- [ ] #54191 <!-- make `AbstractPipe` public -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #53882 <!-- Warn about cycles in extension precompilation -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53286 <!-- Raise an error when using `include_dependency` with
non-existent file or directory -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label May 28, 2024
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024
…JuliaLang#54067)

This makes the following type-stable:
```julia
julia> B = Bidiagonal(rand(3), rand(2), :U);

julia> @inferred (B -> B .* 2)(B)
3×3 Bidiagonal{Float64, Vector{Float64}}:
 0.3929  1.93165   ⋅ 
  ⋅      1.61301  1.00202
  ⋅       ⋅       1.96483
```
Similarly, for other operations involving a single `Bidiagonal` and
numbers. This is not type-stable on master, as the number of
`Bidiagonal` matrices in a broadcast operation is not tracked (even
though this is used in promoting the `uplo`). Since the `uplo` can't be
constant-propagated, we count this by introducing an additional flag in
the promotion mechanism, which is entirely determined by the types of
the terms in the broadcast operation.

---------

Co-authored-by: N5N3 <[email protected]>
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] linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants