Fix compilation and doctests with flintlib 3.2

[tornaria]

Tested with flintlib 3.2-rc1.

May not work with flintlib 3.1.

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.

[tornaria]

@antonio-rojas in case you find this useful when flintlib 3.2 is released.

[antonio-rojas]

Thanks - haven't tested yet, but at first sight it looks like these functions are not used anywhere, can't we just remove the declarations so it works across all flint versions?

[user202729]

how about using abs tol like what you used above?

[tornaria]

I tried but it refuses to work because of the doctest:... above.

[user202729]

Thanks - haven't tested yet, but at first sight it looks like these functions are not used anywhere, can't we just remove the declarations so it works across all flint versions?

But

# This file is auto-generated by the script
#   SAGE_ROOT/src/sage_setup/autogen/flint_autogen.py.
# From the commit 3e2c3a3e091106a25ca9c6fba28e02f2cbcd654a
# Do not modify by hand! Fix and rerun the script instead.

Removing may lead to some confusion (e.g. if some future person want to use the function), and also then we should probably make a blacklist in the autogen file itself.

[user202729]

Feel like flint bug to me? Surely the header should be usable standalone?

[tobiasdiez]

I think it's by design: With
flintlib/flint@3604bb2 one needs to include the gmp header before using fmpz_get_mpz.

[user202729]

Actually the breaking change is flintlib/flint#1964 … looks like backwards incompatibility wasn't intended but…?

[tornaria]

Thanks - haven't tested yet, but at first sight it looks like these functions are not used anywhere, can't we just remove the declarations so it works across all flint versions?

Makes sense, in fact those two functions are deprecated in flintlib 3.2.

A few tests won't work for both versions, because the order of the roots is different. Sorting doesn't work for these. Using set() seems to work but it seems to me it's just by chance.

I can see a few improvements to doctesting may be useful here and there, but I don't have time right now to work on these:

• A tag # set which makes comparision of lists to be up to order (this is tricky to parse, need to balance brackets and it's kind of ill-defined, but maybe a 90% solution is "good enough").
• A tolerance mode # arb tol which will match RBF outputs if the balls intersect. However, using # abs tol eps where eps is the radius of the ball seems good enough. Much better than replacing stuff by ... since most of the doctests are also documentation!
• Fixing the tolerance parsing so it works in cases like the doctest:... above. I suppose it can be assumed that there are no floating point numbers in the ..., but I don't know if this will work ok. Again, a 90% solution may be good enough.

The goals here would be:

• make our lives easier on updating tests to support different versions of dependencies;
• but try to avoid ... so the doctests are still good documentation;
• for the same reason, try to avoid unnatural changes in the output for the sake of doctesting (e.g. sorting, not printing the output, etc)

[user202729]

Using set() seems to work but it seems to me it's just by chance.

Actually…

    if isinstance(val, set):
        itms = sorted_pairs(val)
        return "set([{}])".format(", ".join(itms))

src/sage/doctest/fixtures.py

Yes, using set will just (magically) work.

[user202729]

Actually (2) that isn't the reason. If it were it would have printed out set([...]) instead.

Meanwhile in IPython shell:

In [9]: {*map(str, range(1, 12))}
Out[9]: {'1', '10', '11', '2', '3', '4', '5', '6', '7', '8', '9'}

In [10]: repr({*map(str, range(1, 12))})
Out[10]: "{'3', '2', '7', '6', '10', '5', '11', '1', '9', '4', '8'}"

Probably there's some magic there.

Confusingly dict keys seem to be sorted as well, while in sufficiently new version of Python dict entries are in fact ordered.

---

Edit: apparently the magic is in sage/repl/display/fancy_repr.py. Let's see why.

#: printers for builtin types
_type_pprinters = {
    ...
    dict:                       _dict_pprinter_factory('{', '}'),
    set:                        _set_pprinter_factory('{', '}'),

[…]

some are in SageMath.

def _sorted_dict_pprinter_factory(start, end):
    """
    Modified version of :func:`IPython.lib.pretty._dict_pprinter_factory`
    that sorts the keys of dictionaries for printing.

    EXAMPLES::

        sage: {2: 0, 1: 0} # indirect doctest
        {1: 0, 2: 0}
    """

[tornaria]

Quick comments:

• dict keys in python are kept in the order they were inserted (by spec) not sorted so it may change if the algorithm used to create the dict changes
• set elements are unordered and will be iterated in a non-deterministic order
• elements of CBF can't be sorted, so sorted((x^4 - 1/3).roots(multiplicities=False)) won't work
• sure sorted((x^4 - 1/3).roots(multiplicities=False), key=repr) works,, is this what set output uses?

In any case, my suggestion of the # set tag was so that we don't have to change the example code or the output, so example code matches what a user would do and output matches what sagemath would output. But I guess if set() works (by magic is ok, by chance is not) that's easy enough for now.

[user202729]

that's what I mean, because of some magic (displayhook) in IPython and sage, dict and set are always sorted (first tried by element, if fail it retry by key=str), so it should be good.

[tornaria]

that's what I mean, because of some magic (displayhook) in IPython and sage, dict and set are always sorted (first tried by element, if fail it retry by key=str), so it should be good.

Ok, I added set() to the roots tests. However, there can still (very occasionally) be some trouble:

$ sage -t --random-seed=270965316314199500024584501399721890471 src/sage/rings/complex_arb.pyx
[...]
Failed example:
    set((x^4 - 3).roots(ComplexIntervalField(100), multiplicities=False))
Expected:
    {-1.31607401295249246081921890180? + 0.?e-37*I,
     0.?e-37 + 1.31607401295249246081921890180?*I,
     0.?e-37 - 1.31607401295249246081921890180?*I,
     1.31607401295249246081921890180? + 0.?e-37*I}
Got:
    {-1.31607401295249246081921890180? + 0.?e-37*I,
     0.?e-37 - 1.31607401295249246081921890180?*I,
     0.?e-37 + 1.31607401295249246081921890180?*I,
     1.31607401295249246081921890180? + 0.?e-37*I}
[...]

The problem here seems to be that ComplexIntervalField allows for sorting but it's not sound:

sage: x = PolynomialRing(CBF, 'x').gen()
sage: _, a, b, _ = sorted((x^4 - 3).roots(ComplexIntervalField(100), multiplicities=False))
sage: sorted([a, b])
[0.?e-37 + 1.31607401295249246081921890180?*I,
 0.?e-37 - 1.31607401295249246081921890180?*I]
sage: sorted([b, a])
[0.?e-37 - 1.31607401295249246081921890180?*I,
 0.?e-37 + 1.31607401295249246081921890180?*I]

[user202729]

Ugh, I guess you can do a set(map(str, …)). add # abs tol for extra measure if you want.

If it's confusing only do it in a TESTS:: block, I suppose.

[tornaria]

Ugh, I guess you can do a set(map(str, …)). add # abs tol for extra measure if you want.

If it's confusing only do it in a TESTS:: block, I suppose.

For that it feels less hacky to do sorted(..., repr). Also, CIF should not be sortable...

Anyway, what's going on with testing here?

[user202729]

What do you mean (the CI failing? it's because of the randstate thing right? flintlib/flint#1964 )

[tornaria]

What do you mean (the CI failing? it's because of the randstate thing right? flintlib/flint#1964 )

No, it doesn't even start building sagemath, AFAICT.

[tornaria]

Now it should be ok for both flintlib 3.1 and 3.2.

[github-actions]

Documentation preview for this PR (built with commit 5e2704a; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[tornaria]

The sorting should be fixed now. The issue is not about complex sorting, but about real sorting:

sage: a, b = RIF(0,1), RIF(1,2)
sage: sorted([a,b])
[1.?, 2.?]
sage: sorted([b,a])
[2.?, 1.?]

🤷

[user202729]

In their defense, it works exactly how it is documented. Interval arithmetic does not satisfy trichotomy.

The implementation of complex interval arithmetic comparison could probably be improved though…

---

        In the future, complex interval elements may be unordered,
        but or backwards compatibility we order them lexicographically::

[...]

            # Eventually we probably want to disable comparison of complex
            # intervals, just like python complexes will be unordered.
            ## raise TypeError("no ordering relation is defined for complex numbers")

If it is disabled then comparison will fallback to using repr.

[tornaria]

This is ready and it should be easy to review.

[user202729]

In principle, ideally the flint.pxd should be autogenerated from the script.

it's also necessary to fix this part of the script /src/sage_setup/autogen/flint/reader.py to deal with trailing backslash

.. function:: void n_mulmod_and_precomp_shoup(ulong * ab, ulong * ab_precomp, \
                             ulong a, ulong b,                                \
                             ulong a_pr_quo, ulong a_pr_rem, ulong b_precomp, \
                             ulong n)

Edit: I did that in #39530, run on flint latest master. Let's see if tests pass.

Edit: Unfortunately it's not that easy.

Looks like flint_rand_alloc and flint_rand_free will be deprecated in flint 3.2 anyway. As such I think it's easier to just remove the 2 function declarations.

[tornaria]

In principle, ideally the flint.pxd should be autogenerated from the script.

Sure, but maybe we can get this in and later worry about regenerating with newer sources.

[tornaria]

@user202729 since you already had a look into this PR, would you mind doing formally approving it so it gets merged? This will (hopefully) future proof us for when flintlib 3.2 gets released.

[user202729]

Wait a minute, this thing

#define flint_rand_s flint_rand_struct

makes the C compiler considers the struct name flint_rand_struct on 3.1 too? This doesn't break any ABI or user code that names the struct the other way or anything right?

[tornaria]

Thanks for having a look.

Wait a minute, this thing

#define flint_rand_s flint_rand_struct

makes the C compiler considers the struct name flint_rand_struct on 3.1 too? This doesn't break any ABI or user code that names the struct the other way or anything right?

Yes, this makes the preprocessor substitute the old flint_rand_s for the new flint_rand_struct, so we can use the new API with the old flintlib. This is not part of the ABI since the C ABI doesn't take type names into account.

This is generally preferable IMHO, however this is a special case because (a) flintlib 3.2 hasn't been released yet and (b) the only two functions that require this type are deprecated. Therefore, if you prefer we can do this in a different way, either:

• keep using the old flint_rand_s and use the preprocessor so this still works on flintlib 3.2, or
• eliminate the type and the two deprecated functions

I think the last one might be the better choice, but I can do it either way if you prefer to the current approach.

[user202729]

Okay, either way is good for me as well.

[tornaria]

Thanks!