Fix random polynomial bias

[grhkm21]

For a polynomial ring R say GF(11)["x"], before R.random_element is very far from random, now it is uniformly random.

[grhkm21]

Actually, I think the API is too confusing (it is to myself right now). I will rewrite it tomorrow. Sorry for the confusion.

[tscrim]

This is now even more complicated to make something that is actually wrong be a returned value. Again, the zero polynomial is not monic. It should not be returned when monic=True, ever. It is a contradiction. Remove the _allow_zero, then apply fire to it. Then repeat for everything related to this.

Additionally, I see no point in having other methods that simply are setting options in another method. That is why we have arguments, right? These methods are creating extra maintenance complications and polluting tab completion IMO.

[grhkm21]

FYI, I want monic_or_zero because of my work on jacobians of hyperelliptic curves. There, a valid "x coordinate" of an element is exactly either a random element or zero. I suppose I will have to write this now?

total_possible = 1 + sum(q^i for i in range(g + 1))
if randrange(total_possible) == 0:
    u = R(0)
else:
    u = R.random_element(degree=(0, g)).monic()

And this doesn't even work for say function fields

[tscrim]

FYI, I want monic_or_zero because of my work on jacobians of hyperelliptic curves. There, a valid "x coordinate" of an element is exactly either a random element or zero. I suppose I will have to write this now?

Putting aside that calling a random output an exact computation is a bit strange, assuming you meant a random monic polynomial or zero, if random_element(monic=True) does not return zero (again, as I think it should), then it would make sense to have a random_monic_or_zero() (not quite sure what the best name would be). There isn’t a method with the appropriate arguments. That being said, it probably is most natural to have an additional argument code-wise as you have. (Note, argument names with leading underscores are not hidden). Perhaps the most reasonable thing would be to state that if allow_monic_zero (again, not quite sure on the name, but it should be more descriptive than allow_zero, which will be confusing) is True, then random monic elements can return zero. However, I still find this whole business somewhat strange from an outsiders POV (i.e., without knowing your specific use-case, but that is almost certainly outside my area of expertise).

[grhkm21]

I'll make a _random_element method instead and hide the allow_monic_zero method there, since as you said it's not useful for normal users, and that'll be hidden in tab completion.

[tscrim]

I am still somewhat confused by your use-case for having random monic polynomials and zero (with seemingly some prescribed uniform-like (used very loosely here) distribution). I have convinced myself more that there should not be an extra parameter for monic=True to also return the zero polynomial, but if there is a good use-case for also returning 0, then we can add a carefully documented parameter. Can you explain a little bit more detail how you plan to use this?

[grhkm21]

Actually, I just discussed this with @yyyyx4 and I think we don't actually need the $0$ case, as I had some concepts mixed up. So I think the suggestion for $0$ can be ignored. The current commits should contain no $0$ options, so it's ready for review.

[tscrim]

Thanks. I am glad things are clarified.

Let me remind you about my comment that having methods that simply call another with a default is not good practice. You already have the argument, it makes the code more complicated with more to maintain (and potential to cause conflicts), and pollutes tab completion. random_monic_polynomial should be removed.

You also need to include a check if monic=True and the only degree(s) possible are -1. Related, if degree[1] < -1 then you also need to error out.

I don’t find it useful to have a # random output, but instead to test the desired properties of the output polynomial (e.g., the degree is within the correct range).

[grhkm21]

I have made changes according to your review, thanks! Regarding your review

I don’t find it useful to have a # random output, but instead to test the desired properties of the output polynomial (e.g., the degree is within the correct range).

I actually find it to be the opposite. It's useful for the user to have a quick idea what the output looks like as a mental picture, and for me it makes me read the documentations faster. So I kept those.

[yyyyx4]

I don’t find it useful to have a # random output, but instead to test the desired properties of the output polynomial (e.g., the degree is within the correct range).

I actually find it to be the opposite. It's useful for the user to have a quick idea what the output looks like as a mental picture, and for me it makes me read the documentations faster. So I kept those.

This seems to capture precisely the difference between an example and a test: Examples are for users, tests are for the machine.
(However, there is a non-empty intersection between the concepts — of course examples should work, and on the other hand it may be helpful to users to see some expected/guaranteed properties of the output spelled out.)

[tscrim]

I don’t find it useful to have a # random output, but instead to test the desired properties of the output polynomial (e.g., the degree is within the correct range).

I actually find it to be the opposite. It's useful for the user to have a quick idea what the output looks like as a mental picture, and for me it makes me read the documentations faster. So I kept those.

IMO, this is a fallacy. If I happen to get ``0`, this does not look like a degree 10 polynomial. This is different than if the output order is random (in which case, there are generally ways to convert the test into something deterministic; e.g., sorting).

This seems to capture precisely the difference between an example and a test: Examples are for users, tests are for the machine. (However, there is a non-empty intersection between the concepts — of course examples should work, and on the other hand it may be helpful to users to see some expected/guaranteed properties of the output spelled out.)

Indeed, the last part of what you said is the key point, and not only for the test.

[tscrim]

I have made changes according to your review, thanks! Regarding your review

This isn't quite accurate. Let me be very clear (sorry for the directness, don't read this as me being upset): The random_monic_element method needs to be removed for this to gain a positive review. I see no benefits to keeping it but lots of demerits.

[grhkm21]

I have a question about the failing doctest. The output from sage -t and from the Sage REPL is different. This is from sage -t (also from GitHub actions):

This is from REPL:

[tscrim]

I have a question about the failing doctest. The output from sage -t and from the Sage REPL is different. This is from sage -t (also from GitHub actions):

This happens for a few things; we sort certain outputs for tests that we wouldn’t otherwise (in order to have the consistency in testing the output). So just take the output of sage -t.

[grhkm21]

Interesting, I have done that.

…

On 10 Feb 2024, 23:13 +0000, Travis Scrimshaw ***@***.***>, wrote: > I have a question about the failing doctest. The output from sage -t and from the Sage REPL is different. This is from sage -t (also from GitHub actions): This happens for a few things; we sort certain outputs for tests that we wouldn’t otherwise (in order to have the consistency in testing the output). So just take the output of sage -t. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[grhkm21]

Just a side note, I recently learned about FLINT, and in particular its nmod_poly_randtest_monic method and others, docs. It should be faster and should be used I guess.

[tscrim]

It would be faster, but only for certain base rings (which already have specialized implementations IIRC). So it could be used in those specialized classes.

Also there are 2 doctests failing in polynomial_ring.py according to the test bot.

[grhkm21]

From what I understand Zmod(10)["x"] doesn't have a specialised implementation and uses nmod_poly as the underlying implementation. But that can be deferred for later work.

Oh lmao my samples = [...] is marked as # long time, but the assert check afterwards is not. In fact the --all --long tests pass.

Is there a way to mark the entire test as long time? Or do I just add the tag after every line?

[tscrim]

If you add sage: # long time to the start of a block, then all tests within that block are counted as # long time, but it is generally better to do it locally IMO. In this case, there is only 2 tests it seems.

[tscrim]

Thank you. One last little thing to do. It would also be good if the lines you changed were at 80 char/line or less too.

[tscrim]

Please revert. The indentation is useful and such a change can just create (merge) conflicts.

[grhkm21]

Sorry, I blame this on my editor.

[grhkm21]

I thought the maximum line width in Sage is 100, and (if I didn't miss any) my changes should wrap at that

[tscrim]

For code, we try to do 100 (but there can be reasons for breaking this and this is less enforced), but docstrings we generally try to keep shorter.

[grhkm21]

I will be back and fix the rest in a few hours.

[grhkm21]

Hopefully this is ready now

[github-actions]

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

[tscrim]

Thanks. This is good with me now.

Testing output can be a bit more lax about it, but it is still a good approximate guide.