Include random_element() method to LaurentPolynomialRing

[GiacomoPope]

This PR is an attempt to fix issue #37454 to add a method for random sampling for the LaurentPolynomialRing class which is causing failure in some random testings.

This is not an object I have much familiarity with, but essentially I have followed the usual random sampling for multivariate polynomial rings and tried to follow the existing function name and arguments.

sage: R = LaurentPolynomialRing(QQ, 2, 'x')
sage: R.random_element()
-2*x0*x1^-1 - 2*x0^-2*x1^2 + 5/2*x0^-1 + x0^-2*x1
sage: R.random_element(-5, 0)
7/61*x0^-3*x1^-4 + 1/3*x0^-4*x1^-4 - 1/2*x0^-5*x1^-4
sage: R.random_element(-5, 10)
1/31*x0^-2*x1^7 - 7*x0^-5*x1^10 + 4*x0^2*x1^-2 + 1/3*x0^-2*x1 - 3/44*x0*x1^-5

Included in the doctests is some random testing which tests the properties I have tried to include in this function:

        TESTS::
            sage: rings = [QQ, ZZ, GF(13), GF(7^3)]
            sage: for ring in rings:
            ....:     d = randint(1, 5)
            ....:     R = LaurentPolynomialRing(ring, d, 'x')
            ....:     for _ in range(100):
            ....:         n, m = randint(0, 10), randint(0, 10)
            ....:         f = R.random_element(-n, m)
            ....:         for x in R.gens():
            ....:             assert f.degree(x) <= m
            ....:             assert f.degree(x) >= -n

Fixes #37454

[grhkm21]

small changes

[GiacomoPope]

I have kept the additional argument which were suggested to be pruned as these seem to be used by people who like multivariate polynomial rings so maybe they are interesting here.

[grhkm21]

I still think the arguments should be removed, but I don't have strong opinions.

[GiacomoPope]

I think if someone wants to use this function but is unaware of the underlying randomness function then the extra arguments help the user. But I can also see the reason to remove them.

[grhkm21]

I think if someone wants to use this function but is unaware of the underlying randomness function then the extra arguments help the user. But I can also see the reason to remove them.

Yeah but the same can be said for e.g. QQ["x"].random_element(den_bound=100) or ZZ["x"].random_element(x=-10,y=10), you just have to know they exist. But yeah I don't mind keeping it, and there's doctests

[GiacomoPope]

I suppose the difference there is you can construct these rings with many different base rings / fields, but the underlying polynomial ring is always a multivariate polynomial ring (even for the case with one generator haha)

[tscrim]

It is very confusing to the have degree referring to the valuation. Essentially degree and valuation are opposite ends of the allowable support (in the standard monomial basis). So we should change the argument names.

You're wrong about the multivariate polynomial rings:

sage: L.<x> = LaurentPolynomialRing(QQ)
sage: L.polynomial_ring()
Univariate Polynomial Ring in x over Rational Field

This comes from the construction of polynomial rings (there is a cautionary note about this in that doc IIRC). You should add some examples using this.

The only reason I can think of for keeping the arguments is that this method then sets a default value. However, it does force all underlying implementations to handle these arguments. So I would be in favor of absorbing them into *args and **kwds.

You should also add some examples over more "exotic" base rings to catch more implementations. For example

sage: s = SymmetricFunctions(QQ).s()
sage: L.<x> = LaurentPolynomialRing(s)
sage: type(L.polynomial_ring())
<class 'sage.rings.polynomial.polynomial_ring.PolynomialRing_integral_domain_with_category'>
sage: L.<x,y,z> = LaurentPolynomialRing(s)
sage: type(L.polynomial_ring())
<class 'sage.rings.polynomial.multi_polynomial_ring.MPolynomialRing_polydict_with_category'>

Lastly, the code comment change still is needed.

[grhkm21]

I won't give positive reviews anymore :)

[tscrim]

Oh the melodrama. faints :P

But more seriously, I do appreciate the work you (both!) are putting into helping improve Sage. I'm just picky and overly pedantic about some things. >.> <.<

[grhkm21]

I will clarify that what I meant is I don't have enough experience with reviewing to make sure it's good for Sage, so I won't give them out so easily :)

Oh the melodrama. faints :P

Not sure what that means.

[GiacomoPope]

@tscrim I have removed my complication and now return something very similar to what you suggest above. Thank you for the pointer to IntegerVectors which is essentially what my other function was needing.

I have adjusted the tests and they all pass.

In my opinion this is "fine" and if someone uses this function in the future and needs more out of it, they can adjust it?

[tscrim]

@mantepse Yes, that’s correct. @nbruin is giving an algebro-geometric description. I think we are all okay now with calling the method valuation and the argument name for the random element as such.

@GiacomoPope It would be good to also have a test/example where the min valuation and max degree are both positive and both negative. It is covered by the random choices, but it is better to make sure it is tested every time. Otherwise I think this is good for now.

[GiacomoPope]

Thank you for another review, I really appreciate it. I should have captured everything you mentioned and also hardcoded some of the bounds in doctests to allow both positive/negative.

[tscrim]

Thank you. Just a few last details.

[nbruin]

@mantepse: Apologies for this relatively off-topic tangent. There are some elements here that may be good to get addressed in the documentation concerning the "valuation" terminology, though, so I think there is relevance the larger sagemath context of this ticket. Plus, you're asking :-)

Isn't this "relatively natural valuation" simply the minimal total degree?

Yes. The issue I had is that multivariate power series rings are not discrete valuation rings. That doesn't say they don't admit valuations, but it makes one suspicious if there is a valuation method that doesn't take an argument to specify which valuation is to be taken. Indeed, as we see, the field of fractions of k[[x_1,...,x_n]] does have a valuation on it that corresponds to minimal total degree, but the ring of elements of non-negative valuation is considerably bigger than k[[x_1,...,x_n]]. As I described, there is an algebraic-geometric interpretation of where valuations on function fields are coming from and I did identify what that means for the minimal total degree. That one is in hindsight clear to me, but it was certainly not when I started thinking about it. So a hint onthe justification of the name "valuation" may be appropriate (for discoverability as well ... I wonder if it is the best name if its justification is not so obvious)

The terminology on Laurent polynomial rings is clearly all directly derived from what one gets for power series rings. That's why I'm looking there for justification. Given that Laurent polynomial rings are just subrings of rational function fields, the source for valuations is quite different (richer actually), so it's even stranger that Laurent polynomial rings would have a distinguished valuation on them. Hence, I think the above considerations apply to Laurent polynomial rings as well.

sorry, that's a bit too abstract for me - at least right now. (is it really "a field of fractions"? - i.e., could there be several different fields of fractions?)

No, that's fine. There is a canonical way to construct a field of fractions for an integral domain. Giventhat a multivariate power series ring can just be constructed in an iterative way, by taking k[[x_1]][[x_2]]...[[x_n]], a natural construction is
k((x_1)((x_2))...((x_n)) [which would be quite horrible to compute in]. Computationally there are big differences between the various constructions, particularly because one would have to compute with truncations much of the time. But mathematically, they represent the same object.

Note that this last construction does have a fairly obvious valuation on it (valuation wrt. x_n), but that is not the one that multivariate laurent polynomial rings use!

[GiacomoPope]

If you would like some of the above included in the docstrings in some (condensed) way, I'm happy to make the edit but I need guidance on what would be useful in this context. My work was really only meant to generalise in some way what we had in the univariate case for testing the random element but it seems as thought there's much more maths here than I appreciated.

[nbruin]

this makes me think that an index of a generator would also be valid input. It's much more direct.

Argument against that would be that in principle, any irreducible polynomial could be valid input, since it would describe a prime divisor at which a valuation can be computed. Whether that needs to be implemented is another matter.

[GiacomoPope]

Hmm this is a good point. I think if we do this for valuation() then we must also do this for degree() which has an almost identical form.

I'm happy to do the change which allows x to be either a generator or index. Does anyone else have an opinion?

[tscrim]

Given how degree() is implemented, I would vote (at least for now) to leave it as it is currently implemented.

[tscrim]

@nbruin Thank you for the detailed explanations.

@GiacomoPope It was clearly good for us to have this discussion as we clarified some of the mathematics and improved the documentation. Unfortunately it wasn't quite as simple of a fix as expected, but it's better for it. :)

[GiacomoPope]

Aside from the question about modifying degree (which could/should be a new PR) does anything else need work?

[tscrim]

I don't think so. If @nbruin @grhkm21 @mantepse or someone else has any other changes they want, feel free to revert the positive review.

[tscrim]

@GiacomoPope Sorry, one extra thing (for a followup PR that would depend on this). We should also take into account the grading for the valuation with a std_grading flag...

[GiacomoPope]

Oh yeah, this should indeed follow the way degree behaves... I don't think I'll have time today but if you wanted to do the PR I could maybe review it tonight.

To make matters worse the degree for this case is also broken:

sage: R.<x, y> = LaurentPolynomialRing(ZZ, order=TermOrder("wdegrevlex", [1, 3]))
sage: R(x).degree()
1
sage: R(y).degree()
3
sage: R(1/x).degree()
-1
sage: R(1/y).degree()
-1

It just doesnt deal with negative exponents properly so in fact the weighting for LaurentPolynomialRing is just broken.

EDIT

I made an issue: #37568

[github-actions]

Documentation preview for this PR (built with commit dede9b1; changes) is ready! 🎉