Add quo for LaurentMPolyWrapRing

[JohnAAbbott]

Implemented first version of quo for Laurent polynomial rings.
Some notes:

1. FIXED preimage does not work; you must use the field section instead
2. FIXED There is just one "minimal" test currently
3. Documentation?

[thofma]

Use MapFromFunc instead of the other map_with... thingy. See ?MapFromFunc (this supports preimage and is used everywhere).

P.S.: There is also a morphism type for laurent polynomial rings (created via hom(R, ...)), but preimage seems to not work, so MapFromFunc seems reasonable for the time being.

[JohnAAbbott]

In the tests there is an "ideal membership" check: this uses the function in because the function ideal_membership did not work with the given inputs. Is this the intended behaviour?

[JohnAAbbott]

Use MapFromFunc instead of the other map_with... thingy. See ?MapFromFunc (this supports preimage and is used everywhere).

Code modified -- thanks for the hint.

[thofma]

I think in is more idiomatic, so the tests using it makes sense. I don't mind too much about ideal_membership.

[JohnAAbbott]

I have identified some "infelicities":

1. A value such as x*y^(-2) prints out awkwardly: namely, x1*x2^-1^2; the names have changed, and that "double exponent looks weird!. I wonder whether it may help to choose other names for the "inverse variables" (e.g. inv_x or x_inv)
2. 1/value does not work in the quotient: it gives StackOverflowError
3. It might be nice to use an elimination ordering in the ring representing the quotient so that normal forms tend to prefer expressions involving the "non-inverse variables"

[JohnAAbbott]

I can implement some "better" names (inside function _polyringquo in file src/Rings/Laurent.jl). Here are two possible variants:
This first variant is similar to what a laurent_poly_ring prints:

julia> R, (x, y) = laurent_polynomial_ring(QQ, [:x, :y]);
julia> I = ideal(R, [x^4+1,y^4+1]);
julia> Q,phi = quo(R,I);
julia> phi(x)
x
julia> phi(x^(-1))
x^-1
julia> phi(x^(-2))
x^-1^2
julia> phi(x^(-2)+y)
y + x^-1^2
julia> ans^6
20*x^-1^2*y^-1 - 14*y^-1^2 + 14

This variant is possibly more readable but differs rather from what a laurent_poly_ring prints:

julia> R, (x, y) = laurent_polynomial_ring(QQ, [:x, :y]);
julia> I = ideal(R, [x^4+1,y^4+1]);
julia> Q,phi = quo(R,I);
julia> phi(x)
x
julia> phi(x^(-1))
inv_x
julia> phi(x^(-2))
inv_x^2
julia> phi(x^(-2)+y)
y + inv_x^2
julia> ans^6
20*inv_x^2*inv_y - 14*inv_y^2 + 14

Actually it is probably better to print inv(x) and inv(y) rather than inv_x and inv_y...
Feedback?

[JohnAAbbott]

Here is the third variant mentioned at the end of my previous post:

julia> R, (x, y) = laurent_polynomial_ring(QQ, [:x, :y]);
julia> I = ideal(R, [x^4+1,y^4+1]);
julia> Q,phi = quo(R,I);
julia> phi(x)
x
julia> phi(x^(-1))
inv(x)
julia> phi(x^(-2))
inv(x)^2
julia> phi(x^(-2)+y)
y + inv(x)^2
julia> ans^6
20*inv(x)^2*inv(y) - 14*inv(y)^2 + 14

[joschmitt]

2. `1/value` does not work in the quotient: it gives `StackOverflowError`

This should be resolved by #4862.

[fingolfin]

I think naming the variables inv(x) etc. is a reasonable plan, go ahead with that.

Regarding the "elimination ordering": that also sounds sensible, if we can do it with reasonable effort. But this could also come in a follow-up PR. So unless you can do it "immediately", I'd say focus on the variable name change, and on cleaning up the PR (semicolons etc.) so we can merge it.

[JohnAAbbott]

2. `1/value` does not work in the quotient: it gives `StackOverflowError`

This should be resolved by #4862.

I confirm that this is now fixed -- thanks @joschmitt

[JohnAAbbott]

Regarding the "elimination ordering": that also sounds sensible, if we can do it with reasonable effort. But this could also come in a follow-up PR. So unless you can do it "immediately",....

I've tried a few ideas, but nothing worked for using a "better" ordering. Most likely there is no universally good solution.

[JohnAAbbott]

Test suite revised in accordance with the comments above.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.54%. Comparing base (5242096) to head (7383866).
Report is 96 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4850      +/-   ##
==========================================
+ Coverage   84.82%   85.54%   +0.72%     
==========================================
  Files         683      697      +14     
  Lines       91935   104251   +12316     
==========================================
+ Hits        77980    89178   +11198     
- Misses      13955    15073    +1118     

Files with missing lines	Coverage Δ
src/Rings/Laurent.jl	88.33% <100.00%> (+1.06%)	⬆️

... and 115 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lgoettgens]

I think this is just waiting for a final review @thofma @fingolfin

[thofma]

Fine with me. Would be interesting to understand why we have to use the MapFromFunc crutch and can't use the proper type for maps from Laurent polynomial rings, but this can be investigated later.

[fingolfin]

Fine with me. Would be interesting to understand why we have to use the MapFromFunc crutch and can't use the proper type for maps from Laurent polynomial rings, but this can be investigated later.

Then someone (@JohnAAbbott ?) should open an issue about that else it'll be forgotten...