Resolve some issues of unbound type parameters #3060
Conversation
lgoettgens
added
WIP
lgoettgens
changed the title
|
This is done for now. All remaining instances have to do with |
lgoettgens
force-pushed
the
lg/unbound_args
branch
from
96126ec
to
50ab8dc
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3060 +/- ##
=======================================
Coverage 80.49% 80.50%
=======================================
Files 523 521 -2
Lines 70413 70413
=======================================
+ Hits 56677 56683 +6
+ Misses 13736 13730 -6
|
| function invariant_ring(R::MPolyDecRing, m::MatrixElem{T}, ms::MatrixElem{T}...) where {T} | ||
| return invariant_ring(R, [m, ms...]) |
There was a problem hiding this comment.
Dumb question: is this faster/better/prettier than what was there before?
Looking at this signature, I would now expect that m and ms have different meanings in this function and not that they are just put together in the same vector.
(If it is only about the T we could do
| function invariant_ring(R::MPolyDecRing, m::MatrixElem{T}, ms::MatrixElem{T}...) where {T} | |
| return invariant_ring(R, [m, ms...]) | |
| function invariant_ring(R::MPolyDecRing, matrices::MatrixElem...) | |
| return invariant_ring(R, collect(matrices)) |
as the T is not used... If somebody puts in matrices of different types, this will error if the entries cannot be cast in the coefficient ring of the polynomial ring.)
There was a problem hiding this comment.
This is about the existence of a dispatch where the T is not instantiated.
Your suggestion would be (mostly) fine as well. The single drawback is, that the type of collect(matrices) gets inferred as Vector{Any} for the 0-arg case.
There was a problem hiding this comment.
(I think we should actually throw a meaningful error if there are no matrices, but that's off-topic for now.)
I can't say I can see the ramification of this function not having a completely well-defined return type. I find the new signature confusing (for the human); if it is less confusing for the computer, that's fine by me.
|
The documentation build is still failing, probably one of the signatures there needs to be updated: |
lgoettgens
force-pushed
the
lg/unbound_args
branch
from
8612362
to
7362d94
Compare
lgoettgens
force-pushed
the
lg/unbound_args
branch
from
7362d94
to
26ffc62
Compare
* Track number of unbound type parameters
Once all of them are resolved, we can enable the Aqua check
unbound_args.Most fixes were just minor reordering of a method header, such that the type parameter no longer gets instantiated. In some cases with varargs, it seemed more reasonable to split the method into two (one with 0 args, and one with at least 1 arg).