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

Don't access parent of triangular matrix in powm #52583

Merged
merged 3 commits into from
Dec 25, 2023
Merged

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Dec 19, 2023

Since the values stored in the parent corresponding to the structural zeros of a tridiagonal matrix aren't well-defined, using it in ldiv! is a footgun that may lead to heisenbugs (one seen in https://buildkite.com/julialang/julia-master/builds/31285#018c7cc7-6c77-41ac-a01b-1c7d14cb1b15). This PR changes it to using the tridiagonal matrix directly in ldiv!, which should lead to predictable results, and be bug-free. The failing tests for #52571 pass locally with this change.

@jishnub jishnub added domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug labels Dec 19, 2023
@dkarrasch
Copy link
Member

I'm not familiar with this part of the code. Why is this method restricted to BlasFloat to begin with, but not to StridedMatrix? Can we relax the restriction? Is there a way to test the difference? And finally, should this be backported to v1.10?

@jishnub jishnub added backport 1.9 Change should be backported to release-1.9 backport 1.10 Change should be backported to the 1.10 release labels Dec 22, 2023
@jishnub
Copy link
Contributor Author

jishnub commented Dec 22, 2023

I've relaxed the eltype, which now makes exponentiation work with a BigFloat upper-triangular matrix (used to fail earlier as the eltype was too narrow).

@jishnub
Copy link
Contributor Author

jishnub commented Dec 22, 2023

Why are the tests not running, btw? I see two successful checks (typos and labels), but no actual testsets being run

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jishnub jishnub merged commit ef549ae into master Dec 25, 2023
2 checks passed
@jishnub jishnub deleted the jishnub/tripowm branch December 25, 2023 12:51
KristofferC pushed a commit that referenced this pull request Jan 5, 2024
Since the values stored in the parent corresponding to the structural
zeros of a tridiagonal matrix aren't well-defined, using it in `ldiv!`
is a footgun that may lead to heisenbugs (one seen in
https://buildkite.com/julialang/julia-master/builds/31285#018c7cc7-6c77-41ac-a01b-1c7d14cb1b15).
This PR changes it to using the tridiagonal matrix directly in `ldiv!`,
which should lead to predictable results, and be bug-free. The failing
tests for #52571 pass locally with this change.

(cherry picked from commit ef549ae)
@KristofferC KristofferC mentioned this pull request Jan 5, 2024
33 tasks
KristofferC pushed a commit that referenced this pull request Jan 5, 2024
Since the values stored in the parent corresponding to the structural
zeros of a tridiagonal matrix aren't well-defined, using it in `ldiv!`
is a footgun that may lead to heisenbugs (one seen in
https://buildkite.com/julialang/julia-master/builds/31285#018c7cc7-6c77-41ac-a01b-1c7d14cb1b15).
This PR changes it to using the tridiagonal matrix directly in `ldiv!`,
which should lead to predictable results, and be bug-free. The failing
tests for #52571 pass locally with this change.

(cherry picked from commit ef549ae)
KristofferC added a commit that referenced this pull request Feb 6, 2024
Backported PRs:
- [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't
throw -->
- [x] #52583 <!-- Don't access parent of triangular matrix in powm -->
- [x] #52645 <!-- update --gcthreads section in command line options -->
- [x] #52423 <!-- update nthreads info in versioninfo -->
- [x] #52721 <!-- inference: Guard TypeVar special case against vararg
-->
- [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g.
devved in an environment higher in the load path -->
- [x] #52752 <!-- staticdata: handle cycles in datatypes -->
- [x] #52758 <!-- use a Dict instead of an IdDict for caching of the
`cwstring` for Windows env variables -->
- [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages -->
- [x] #52994 <!-- place work-stealing queue indices on different cache
lines to avoid false-sharing -->
- [x] #53015 <!-- Add type assertion in iterate for logicalindex -->
- [x] #53032 <!-- Fix a list in GC devdocs -->
- [x] #52748 
- [x] #52856 
- [x] #52878
- [x] #52754 
- [x] #52228
- [x] #52924
- [x] #52569 <!-- Fix GC rooting during rehashing of iddict -->
- [x] #52605 <!-- Default uplo in symmetric/hermitian -->
- [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to
fix unrooted nodes -->
- [x] #52781 <!-- fix type-stability bugs in Ryu code -->
- [x] #53055 <!-- Profile: use full terminal cols to show function name
-->
- [x] #53096 
- [x] #53076 
- [x] #52841 <!-- Extensions: make loading of extensions independent of
what packages are in the sysimage -->
- [x] #52078 <!-- Replace `&hArr;` by `&harr;` in documentation -->
- [x] #53035 <!-- use proper cache-line size variable in work-stealing
queue -->
- [x] #53066 <!-- doc: replace harr HTML entity by unicode -->
- [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our
defines to match -->
- [x] #53121 

Non-merged PRs with backport label:
- [ ] #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.10 Change should be backported to the 1.10 release label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.9 Change should be backported to release-1.9 domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants