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

Further solving related adjustments #1598

Merged
merged 8 commits into from
Feb 9, 2024
Merged

Conversation

joschmitt
Copy link
Collaborator

I learned that nullspace(::RingElement) returns a "rational nullspace" EXCEPT for ZZMatrix.
The output of Solve.kernel is still not exactly the same as of kernel; I will probably adjust some more.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cd38e14) 87.04% compared to head (4b6c5e5) 87.02%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1598      +/-   ##
==========================================
- Coverage   87.04%   87.02%   -0.03%     
==========================================
  Files         115      115              
  Lines       29593    29603      +10     
==========================================
+ Hits        25760    25762       +2     
- Misses       3833     3841       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Solve.jl Outdated Show resolved Hide resolved
Now `kernel(zero_matrix(ZZ, 2, 2))` gives
```
[1   0]
[0   1]
```
which certainly looks nicer than
```
[0   1]
[1   0]
```
@joschmitt
Copy link
Collaborator Author

joschmitt commented Feb 8, 2024

Regarding the structure of the kernel, which do you prefer? M is a ZZMatrix.

julia> kernel(M, side = :right)
[ 1    0    0    0]
[ 0    1    0    0]
[ 0    0    1    0]
[ 0    0    0    1]
[-2   -1    0   -2]
[ 1    0   -1    1]

julia> kernel(change_base_ring(QQ, M), side = :right)
[ 1   -1    0    1]
[-2    0   -1   -2]
[ 1    0    0    0]
[ 0    1    0    0]
[ 0    0    1    0]
[ 0    0    0    1]

julia> kernel(change_base_ring(GF(17), M), side = :right)
[ 1   16    0    1]
[15    0   16   15]
[ 1    0    0    0]
[ 0    1    0    0]
[ 0    0    1    0]
[ 0    0    0    1]

julia> kernel(M, side = :left)
[1   0   0   0   -2    1]
[0   1   0   0   -1    0]
[0   0   1   0    0   -1]
[0   0   0   1   -2    1]

julia> kernel(change_base_ring(QQ, M), side = :left)
[ 1   -2   1   0   0   0]
[-1    0   0   1   0   0]
[ 0   -1   0   0   1   0]
[ 1   -2   0   0   0   1]

julia> kernel(change_base_ring(GF(17), M), side = :left)
[ 1   15   1   0   0   0]
[16    0   0   1   0   0]
[ 0   16   0   0   1   0]
[ 1   15   0   0   0   1]

Notice that the functions over QQ and GF(17) call flint, so it would be somewhat easier to adjust the ZZ functionality. Then again, there are some instances in Hecke relying particularly on the shape in the ZZ case (although I haven't checked yet whether the 180° flip to get from the ZZ shape to the QQ shape would create a problem there).
EDIT: Apparently QQMatrix is the only type for which we compute kernels in C? (And only for those only since last week.)

@joschmitt joschmitt marked this pull request as ready for review February 9, 2024 15:10
@joschmitt
Copy link
Collaborator Author

This is good to go for now. Would be good if we could make a minor release after it is merged because I need it for Hecke. (I can already adjust the version number here, if you want.)

@thofma
Copy link
Member

thofma commented Feb 9, 2024

Yes, please bump the version number.

@joschmitt
Copy link
Collaborator Author

This should only be breaking because it tests with Oscar on top of my Hecke js/solve branch (I didn't no CI is this "clever"!). So the changes in Hecke are breaking, but not the ones here.

src/Solve.jl Outdated Show resolved Hide resolved
src/Solve.jl Outdated Show resolved Hide resolved
@thofma thofma merged commit 343ce65 into Nemocas:master Feb 9, 2024
11 of 15 checks passed
@joschmitt joschmitt deleted the js/solve branch February 12, 2024 12:21
ooinaruhugh pushed a commit to ooinaruhugh/AbstractAlgebra.jl that referenced this pull request Feb 15, 2024
* Don't call `nullspace` for `RingElem` and add a deepcopy

* Rearrange `kernel` a bit

Now `kernel(zero_matrix(ZZ, 2, 2))` gives
```
[1   0]
[0   1]
```
which certainly looks nicer than
```
[0   1]
[1   0]
```

* Add `Solve.kernel(::Ring, ::MatElem)`

* Do it right: make `side = :left` the default!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants