-
Notifications
You must be signed in to change notification settings - Fork 138
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
Resolve unbound type parameters #4363
Conversation
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.
Missing docstring for
direct_sum(M::ModuleFP{T}...; task::Symbol = :sum) where T
. Check Documenter's build log for details.' in@docs
block in src/CommutativeAlgebra/ModulesOverMultivariateRings/module_operations.md:29-31
I think you need to update the docstring signature at this place as well
This also removes some of the ambiguities for direct_product, direct_sum, tensor_product with empty argument list. Re-enable Aqua test for unbound type parameters.
Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
5372ad2
to
9b802ac
Compare
1: direct sum of (Multivariate polynomial ring in 1 variable over GF(7)^1, Multivariate polynomial ring in 1 variable over GF(7)^1) | ||
2: direct sum of (Multivariate polynomial ring in 1 variable over GF(7)^1, Multivariate polynomial ring in 1 variable over GF(7)^1) | ||
1: direct sum of FreeMod{FqMPolyRingElem}[Multivariate polynomial ring in 1 variable over GF(7)^1, Multivariate polynomial ring in 1 variable over GF(7)^1] | ||
2: direct sum of FreeMod{FqMPolyRingElem}[Multivariate polynomial ring in 1 variable over GF(7)^1, Multivariate polynomial ring in 1 variable over GF(7)^1] |
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 changed docstring is caused by the attribute direct_product
changing from a Tuple
to a Vector
, and it gets printed raw by this function:
function Hecke.show_direct_sum(io::IO, G)
D = get_attribute(G, :direct_product)
D === nothing && error("only for direct sums")
print(io, "direct sum of ")
show(IOContext(io, :compact => true), D)
end
For nicer printing, that function (and its siblings around it) should be improved.
That leave the question whether the change from tuple to vector is fine. I had a quick look at the code using this attribute and I believe it is all fine either. Long term a vector IMO makes more sense there.
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.
Looks fine from my side, if the tests pass.
It seems that most of the relevant changes were for the modules and only revealed in my code because I use them. I'm afraid, I'm not qualified to change the printing of modules myself.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4363 +/- ##
=======================================
Coverage 84.39% 84.39%
=======================================
Files 668 668
Lines 88557 88567 +10
=======================================
+ Hits 74736 74747 +11
+ Misses 13821 13820 -1
|
This also removes some of the ambiguities for direct_product,
direct_sum, tensor_product with empty argument list.
Re-enable Aqua test for unbound type parameters.