-
Notifications
You must be signed in to change notification settings - Fork 63
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
Iterator for minors of matrix #1821
Iterator for minors of matrix #1821
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
end | ||
end | ||
return(mins) | ||
minors(A::MatElem, k::Int) = collect(minors_iterator(A, k)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
Resolves oscar-system/Oscar.jl#4169.
This is an attempt to implement the suggestion from Issue 4169 in the Oscarl.jl repo:
minors_iterator
has been introduced, that constructs an iterator that calculates all minors.minors
has been replaced by a function that collects the minors fromminors_iterator
.The functions still relies on the internal function
combinations
. For further improvements in performance, one could consider turningcombinations
into a generator too.