All monomial for free standard graded modules #2894
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2894 +/- ##
==========================================
- Coverage 80.39% 80.39% -0.01%
==========================================
Files 456 457 +1
Lines 65235 65295 +60
==========================================
+ Hits 52447 52491 +44
- Misses 12788 12804 +16
|
| @@ -6,3 +6,6 @@ include("primary_invariants.jl") | |||
| include("secondary_invariants.jl") | |||
| include("fundamental_invariants.jl") | |||
| include("affine_algebra.jl") | |||
|
|
|||
| # postponed inclusion from src/Modules/Modules.jl due to dependency | |||
| include("../Modules/Iterators.jl") | |||
There was a problem hiding this comment.
This is not really about this PR, but it seems to me like we should start thinking about putting all types ((mutable) structs) in a handful of 'central' places or at least make sure that they can be included at first.
Although in this case we could also just move the all_monomials for rings to the Rings/ directory where it arguably belongs.
There was a problem hiding this comment.
Yes, this discussion has to take place at some point. But not here, eventually.
| for i in 1:r | ||
| d_loc = d - Int(degree(F[i])[1]) | ||
| d_loc < 0 && continue | ||
| result = result + length(all_monomials(R, d_loc)) |
There was a problem hiding this comment.
One should be aware that all_monomials completely ignores the grading of a MPolyDecRing. But you only consider the standard graded case, so it's fine.
|
|
||
| mon_it = all_monomials(R, d_loc) | ||
| res = iterate(mon_it, nothing) | ||
| res === nothing && i == ngens(F) && return nothing |
There was a problem hiding this comment.
Can res === nothing happen? mon_it is an iterator over a component of non-negative degree, so there should always be at least one monomial, no?
There was a problem hiding this comment.
I don't know. Probably not. But the line doesn't hurt.
There was a problem hiding this comment.
But what does happen if res === nothing and i != ngens(F), so the return nothing is not reached?
In the next line, you assign x, s = res which throws an error.
Handling this case properly (increase i etc.) might be possible, but doesn't seem to be worth the effort.
| res === nothing && i == ngens(F) && return nothing | |
| @assert res !== nothing |
Maybe this would be clearer then?
There was a problem hiding this comment.
Yes, you might be right. But I don't want to interrupt the tests again now. Let's first see whether they pass in principle. In either case, the line doesn't seem to be breaking things. Worst case it's confusing to the reader.
|
|
||
| mon_it = all_monomials(R, d_loc) | ||
| res_loc = iterate(mon_it, nothing) | ||
| res_loc === nothing && i == ngens(F) && return nothing |
Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>
Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>
|
From my side this is good to be merged. In my opinion, the small change discussed above is not worth spending the time and (electric) energy to running all the tests again. |
| @testset "all_monomials for graded free modules" begin | ||
| R, (x, y, u, v, w) = QQ[:x, :y, :u, :v, :w] | ||
| S, (x, y, u, v, w) = grade(R) | ||
|
|
||
| F = graded_free_module(S, [-1, 2]) | ||
|
|
||
| for d in -4:4 | ||
| amm = Oscar.all_monomials(F, d) | ||
| @test d < -1 || !isempty(amm) | ||
| @test all(x->degree(x) == grading_group(F)([d]), amm) | ||
| end | ||
| end |
There was a problem hiding this comment.
Why is this identical test in ModulesGraded.jl and in GradedModules.jl?
(By the way, is the latter filename the result of a mishap or intentional?)
There was a problem hiding this comment.
I put the tests in one file and ran it locally. Tests failed completely, but not due to the part I added. I suppose that test-file was deprecated, but not removed. Just to be sure, I then put my tests in both, finally. Somebody should tell us what's up with the test files, though.
As discussed with @jankoboehm : An iterator over all monomials in standard graded free modules. To be used as a backend for eventual implementations of
vector_spaceand friends. For the non-standard-graded case we will need other functionality, but this is not a contradiction. Nothing is exported and what is added fits with the work by @joschmitt .