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.

Sign up for GitHub

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

Jump to bottom

Iterator for minors of matrix #1821

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Iterator for minors of matrix #1821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
626e4b8
Iterator for minors of matrix
oskarhenriksson Oct 1, 2024
3712ce3
Export minors_iterator
oskarhenriksson Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions src/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2494,16 +2494,35 @@ julia> minors(A, 2)

```
"""
function minors(A::MatElem, k::Int)
row_indices = combinations(nrows(A), k)
col_indices = combinations(ncols(A), k)
mins = Vector{elem_type(base_ring(A))}(undef, 0)
for ri in row_indices
for ci in col_indices
push!(mins, det(A[ri, ci]))
end
end
return(mins)
minors(A::MatElem, k::Int) = collect(minors_iterator(A, k))
Copy link
Member

Choose a reason for hiding this comment

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

Should we just replace minors by minors_iterator (i.e. rename the latter to minors) ? Perhaps not because it would be breaking, but other than this (clearly important) concern I see no downsides?

I don't want to hold up this PR for it, but wanted to at least mention it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @fieker or @thofma have opinions on this

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstring of minors states clearly that it returns an array, so I would consider this breaking (at least for AA).
But we could discuss if we consider this breaking for Oscar, or if it is fine to do in a breaking AA release and a minor Oscar release afterwards

Copy link
Member

Choose a reason for hiding this comment

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

It definitely would be breaking, regardless of what the docstring says, because there could be code which assumes it is an array and accesses it as such.

And in fact Oscar has a lot of code looking like this:

  R, (x, y, z, w) = QQ[:x, :y, :z, :w]

  M = R[x y z; y-1 z-2 w]

  I = ideal(R, minors(M, 2))

So I think we should leave it as is (and only consider changing it for "Oscar 2.0" if at all)

Copy link
Contributor Author

@oskarhenriksson oskarhenriksson Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks everyone for looking at this so quickly!

I agree that one should be very careful about changing minors to return an iterator (I have code myself that would break), but I also agree that letting minors and other combinatorially heavy constructions be iterators would make the most sense in the long term.

As for the point that @fingolfin raises above: Perhaps this particular issue can be solved by just allowing users to construct ideals from iterators, rather than just vectors?


@doc raw"""
minors_iterator(A::MatElem, k::Int)

Return an iterator that computes the `k`-minors of `A`.

# Examples

```jldoctest; setup = :(using AbstractAlgebra)
julia> A = ZZ[1 2 3; 4 5 6]
[1 2 3]
[4 5 6]

julia> first(minors_iterator(A, 2))
-3

julia> collect(minors_iterator(A, 2))
3-element Vector{BigInt}:
-3
-6
-3

```
"""
function minors_iterator(M::MatElem, k::Int)
row_indices = combinations(nrows(M), k)
col_indices = combinations(ncols(M), k)
return (det(M[rows, cols]) for rows in row_indices for cols in col_indices)
end

@doc raw"""
Expand Down
1 change: 1 addition & 0 deletions src/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ export matrix_space
export max_fields
export max_precision
export minors
export minors_iterator
export minpoly
export mod
export mod!
Expand Down
Loading