Iterator for minors of matrix

[oskarhenriksson]

Resolves oscar-system/Oscar.jl#4169.

This is an attempt to implement the suggestion from Issue 4169 in the Oscarl.jl repo:

• A function minors_iterator has been introduced, that constructs an iterator that calculates all minors.
• The old function minors has been replaced by a function that collects the minors from minors_iterator.

The functions still relies on the internal function combinations. For further improvements in performance, one could consider turning combinations into a generator too.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.02%. Comparing base (11bbd58) to head (3712ce3).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1821      +/-   ##
==========================================
- Coverage   88.03%   88.02%   -0.01%     
==========================================
  Files         119      119              
  Lines       29993    29988       -5     
==========================================
- Hits        26403    26398       -5     
  Misses       3590     3590              

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

[fingolfin]

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.

[fingolfin]

Maybe @fieker or @thofma have opinions on this

[lgoettgens]

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

[fingolfin]

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)

[oskarhenriksson]

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?