Skip to content

Suboptimal choice of algorithm by Matrix(QQ).solve_right() #39197

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

Closed
2 tasks done
user202729 opened this issue Dec 24, 2024 · 3 comments · Fixed by #39733
Closed
2 tasks done

Suboptimal choice of algorithm by Matrix(QQ).solve_right() #39197

user202729 opened this issue Dec 24, 2024 · 3 comments · Fixed by #39733
Labels

Comments

@user202729
Copy link
Contributor

Steps To Reproduce

set_random_seed(1)
n=26
entry_size = 100
A = matrix.block([
        [matrix([[randint(0, 2^entry_size) for col in range(n)] for row in range(n*2)])],
        ])
x = vector([randint(0, 2^entry_size) for i in range(n)])
b = A*x

%time x1 = A.solve_right(b)
# more than a second

%time x1 = A._solve_right_general(b.column(), check=True).column(0)
# instant

Expected Behavior

both are fast

Actual Behavior

Additional Information

No response

Environment

  • OS: Linux
  • Sage Version: latest develop

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@user202729
Copy link
Contributor Author

set_verbose(5) gives some interesting information.

  • first, somehow the cached pivot value is not reused in the first case.
  • second, there's a bunch of echelon multi-modular possibly not converging in the slow case, but not the fast case.

@user202729
Copy link
Contributor Author

user202729 commented Dec 24, 2024

Looks like the cause is the fact that the ring is changed to QQ somehow.

Which boils down to…

A = matrix(ZZ, [[randint(0, 2^entry_size) for col in range(n*2)] for row in range(n)])
A.pivots()  # fast

A = matrix(QQ, [[randint(0, 2^entry_size) for col in range(n*2)] for row in range(n)])
A.pivots()  # slow

In matrix_rational_dense.pyx:

        if algorithm is None:
            if self._nrows <= 25 or self._ncols <= 25:
                algorithm = 'flint'
            else:
                algorithm = 'multimodular'

Introduced by #22970 . Looks like multimodular clears denominator then runs multimodular algorithm.

@user202729 user202729 changed the title Weird slowdown with solve_right() Suboptimal choice of algorithm by Matrix(QQ).solve_right() Dec 24, 2024
@user202729
Copy link
Contributor Author

Looks like the problem is that the initial guess for the height is way too low.

sage: set_random_seed(1)
sage: num_col = 60
sage: num_row = 30
sage: entry_size = 100
sage: A = matrix(QQ, [[randint(1, 2^entry_size) for col in range(num_col)] for row in range(num_row)])
sage: A.height()
1267283060447949307261369136578
sage: %time A.echelonize(algorithm="multimodular", height_guess=10^2000)
CPU times: user 422 ms, sys: 55 μs, total: 422 ms
Wall time: 425 ms
sage: RR(A.height())
1.92386909048248e904

vbraun pushed a commit to vbraun/sage that referenced this issue Apr 4, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 5, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 7, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 10, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 13, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 13, 2025
sagemathgh-39733: Make rational matrix rref default to flint_multimodular, add suboptions for flint algorithm
    
Because one of the algorithms used by flint is multimodular, it ought to
be faster than the implementation in Python.

At least after we upgrade to a version after
flintlib/flint#2129 .

(p/s: if someone uses the old version, the current choice of flint might
be slower in some cases, see the linked issue. An alternative which is
likely always faster is to explicitly use the multimodular algorithm in
flint. Do you think the current implementation is fine, or should we
provide an explicit `flint_multimodular` option instead?)

Fixes sagemath#39197

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->


sagemath#39204
    
URL: sagemath#39733
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Apr 18, 2025
sagemathgh-39204: Speed up multimodular algorithm in bad case
    
**Edit**: Now flint has fixed
flintlib/flint#2129 , it should be better to
just switch to flint entirely — according to
flintlib/flint#2129 (comment) ,
one of the possible algorithms by flint is multimodular, which should be
faster than or equal to what we're having now. If it is slower in any
case, bug can be reported upstream.

------

Related to sagemath#39197.

Technically the algorithm doesn't deviate from @williamstein  's
original book; however the original book doesn't say *how many*
additional primes to add each time.

The original implementation roughly consider 3 more primes each time.
This can be highly inefficient when there are more columns than rows,
which makes the result's height much higher than the guess.

This increases the length of `M` by roughly a factor of `1.2` each time.
Worst case it makes the algorithm slower by a (hopefully small) constant
factor.

For the added test case, it appears to improve the performance.
(Originally takes 40s, now takes <10s on my machine)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39204
Reported by: user202729
Reviewer(s): Travis Scrimshaw
@vbraun vbraun closed this as completed in 3531a87 Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant