Opened 2 years ago

Closed 2 years ago

#29472 closed defect (fixed)

Modify doctest such that they work with pari >= 2.11.3 and pari < 2.11.3

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.2
Component: packages: standard Keywords:
Cc: mkoeppe, mjo, chapoton Merged in:
Authors: Jonathan Kliem Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 4f86bc3 (Commits, GitHub, GitLab) Commit: 4f86bc37571a3c7912edb94d2976f857b93662b1
Dependencies: Stopgaps:

Status badges

Description

As we allow sage to use a system pari from version 2.11.1 we modify the doctests such that they pass with both pari versions.

All of those failures are due to different choices of representation or to small differences in precision.

Currently, there are failing doctests in debian-sid https://github.com/kliem/sage-test-27122/runs/560405581, which is (correctly) not fixed by #29445 (debian sid uses pari (2.11.4~pre1-1), which contains no serious bug that we are aware of).

Follow up: #29313.

Change History (28)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/29472
  • Commit set to ca4ee7d644ff3ee45b2c6e6c9ae8b5a08c54abda

New commits:

ca4ee7dmodify doctests such that they work with various pari verions

comment:2 Changed 2 years ago by git

  • Commit changed from ca4ee7d644ff3ee45b2c6e6c9ae8b5a08c54abda to 98bfefda186e4c7992f7120b5956b5b41bfdcbb3

Branch pushed to git repo; I updated commit sha1. New commits:

98bfefdsmall fix

comment:3 Changed 2 years ago by gh-kliem

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by gh-kliem

This seems to work.

The failing doctests related to pari for debian sid are gone and the tests still pass e.g. on debian buster:

https://github.com/kliem/sage-test-27122/actions/runs/72102779

comment:5 Changed 2 years ago by gh-kliem

  • Cc mkoeppe mjo added

comment:6 follow-up: Changed 2 years ago by mjo

I'm surprised this works:

TESTS:

We test the above doctest. The representation depends on the PARI version::

    sage: [x] = [K.uniformizer(P) for P,e in factor(K.ideal(7))]
    sage: x in (t^2 + 3*t +1, t^2 - 4*t +1)
    True

I thought that :: cleared the global K. In any case, I would recommend reconstructing K here so that the test doesn't break if someone adds another example to the end of the EXAMPLES: block above it (and likewise for the other "test the above doctest" tests).

comment:7 Changed 2 years ago by git

  • Commit changed from 98bfefda186e4c7992f7120b5956b5b41bfdcbb3 to 855098cb2f1c8d88592c2cb021bdb1bf099121fd

Branch pushed to git repo; I updated commit sha1. New commits:

855098ccreate the context for the tests locally

comment:8 in reply to: ↑ 6 Changed 2 years ago by gh-kliem

I do this in many places, as in

A polyhedron::

    sage: P = polytopes.cube()

The f-vector::

    sage: P.f_vector()
    (1, 8, 12, 6, 1)

However, this can be really annoying when you have tests in the test section that rely on stuff way above. So I changed it. It is also easier to read the tests in that way.

Replying to mjo:

I'm surprised this works:

TESTS:

We test the above doctest. The representation depends on the PARI version::

    sage: [x] = [K.uniformizer(P) for P,e in factor(K.ideal(7))]
    sage: x in (t^2 + 3*t +1, t^2 - 4*t +1)
    True

I thought that :: cleared the global K. In any case, I would recommend reconstructing K here so that the test doesn't break if someone adds another example to the end of the EXAMPLES: block above it (and likewise for the other "test the above doctest" tests).

comment:9 Changed 2 years ago by git

  • Commit changed from 855098cb2f1c8d88592c2cb021bdb1bf099121fd to 4f86bc37571a3c7912edb94d2976f857b93662b1

Branch pushed to git repo; I updated commit sha1. New commits:

4f86bc3fix failing doctest

comment:10 Changed 2 years ago by mjo

This doesn't need to be done any more if the ./configure script is going to reject anything other than a patched 2.11.3.

comment:11 follow-up: Changed 2 years ago by gh-kliem

Was this the decision made to reject 2.11.2 as well? But yes, this ticket is not needed anymore if we only accept a patched 2.11.3 (Deban sid has 2.11.4 beta, which works also fine, but that's not the point). I'm still hoping that distributions will patch their packages, but who knows if that is going to happen.

Also by the automated testing we will catch on, if this is happening and we can still do this ticket then.

Also we really should make some general decisions what kind of doctest failures justify a reject and how we proceed if we don't reject.

comment:12 in reply to: ↑ 11 Changed 2 years ago by mkoeppe

Replying to gh-kliem:

Was this the decision made to reject 2.11.2 as well?

No such decision has been made. I think you should make a decision which versions to support based on your knowledge about the details of these doctests.

comment:13 Changed 2 years ago by gh-kliem

ubuntu eoan has pari 2.11.2-2 and this seems to work perfectly fine with our doctests.

https://github.com/kliem/sage-test-27122/runs/560405517

The doctests in this ticket are all about representation, I have to admit that I trusted our code in some ways:

  • I haven't looked all the way into optimized_representation, but both choices seem fine to me,
  • /src/sage/rings/polynomial/polynomial_quotient_ring.py, S.S_class_group([]): from the documentation I understand that any QQ-base exchange is fine
  • for the global integral model I'm completely relying on is_global_integral_model

So for our doctests pari 2.11.2 or patched pari 2.11.3 work fine (or pari 2.11.4 beta). There are probably other bugs, but I have no clue.

comment:14 Changed 2 years ago by mjo

Since it only takes two people to make a change, and since there are two antipodal views on which changes should be made, it would be good to come to an agreement even if we have to vote on it on sage-devel. I'm de-motivated to work on this stuff when it's just Matthias and I who want to do opposite things, but I would suck it up if the consensus is against me.

The question is, essentially: should we have/add doctests for upstream bugs in/to the sage library?

If yes, we tend to force the latest (and possibly patched) versions of sage's dependencies, because every new version of a dependency likely fixes some math bug that we can add a doctest for. The upside is that we can ensure that users who download and build sage themselves (even if they have to build a bunch of custom SPKGs) get a known quantity.

If no, then we can accept slightly older system packages and sage's test suite will still pass. This saves a lot of time while building sage, but can delay bugfixes until distributions ship them or upgrade their packages. In other words, system dependencies are the responsibility of packagers, but if you don't like that you can use the SPKG instead.

Case in point: this equivalence I posted on #29494 should be symmetric, but isn't.

sage: M1 = matrix(ZZ,[[16,6],[6,10]])
sage: M2 = matrix(ZZ,[[4,3],[3,10]])
sage: Q1 = QuadraticForm(M1)
sage: Q2 = QuadraticForm(M2)
sage: Q1.is_globally_equivalent_to(Q2)
False
sage: Q2.is_globally_equivalent_to(Q1)
True

That bug was fixed in pari-2.11.3. The first view says we should add a doctest for that, and thereafter bump the minimum required version of pari to the latest (patched) 2.11.3. The second view says that the bug is fixed upstream and isn't going to come back, so we don't need to test it in sagelib, and it's OK if users who have installed pari-2.11.2 on their system experience the bug until they upgrade (i.e. we should still let them use 2.11.2 if that's what they have).

This ticket is only relevant in that it emphasizes how doctests are a side-channel for introducing version constraints.

comment:15 Changed 2 years ago by mkoeppe

I also think that a discussion on sage-devel -- on the general topic of the relation of sage-the-distribution and the sage doctests -- would be valuable. The specific decision on pari is not very important to me (though the bug on quadratic forms looks pretty serious to me).

I would suggest to do this right after the 9.1 release, though, not now.

A lot of spkg-configure tickets have been / are being merged in 9.1. Developers who have not followed the last betas should get a chance to familiarize themselves with the technical status of supporting distribution packages in the new release.

comment:16 Changed 2 years ago by gh-kliem

  • Milestone changed from sage-9.1 to sage-9.2
  • Status changed from needs_review to needs_info

Ok, let's wait and see how we decide on things.

comment:17 Changed 2 years ago by mkoeppe

  • Cc chapoton added

comment:18 follow-up: Changed 2 years ago by dimpase

On this one it's probably not too muich work to make tests formatting-independent. I can give it a go, although I'm far from NT, so I might not know what happens in a particular place.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by mjo

Replying to dimpase:

On this one it's probably not too muich work to make tests formatting-independent. I can give it a go, although I'm far from NT, so I might not know what happens in a particular place.

Making the tests more robust is valuable in and of itself, but the only reason this isn't done yet is because we weren't sure if it was going to be a waste of time. I think we should support a few recent versions of a package if there's no API breakage, but Matthias thinks we should add doctests (and ./configure tests) for upstream math bugs, essentially limiting support to the latest version. If we do that, then there's no reason to make the tests flexible, because the ./configure script will have to reject all but the latest version to keep the test suite working.

Aside: I'm terrified to review anything inside this dependency-branch-rebase house of cards.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 2 years ago by mkoeppe

Replying to mjo:

... essentially limiting support to the latest version ...

... or distribution's version with backported patches ...

comment:21 in reply to: ↑ 20 Changed 2 years ago by mjo

Replying to mkoeppe:

Replying to mjo:

... essentially limiting support to the latest version ...

... or distribution's version with backported patches ...

When we backport, it takes at least another 30 days (and requires several other developers to act) before those changes hit the stable tree, so that's not as simple as it sounds. But more to the point, I have better things to do than backport patches for hundreds of packages and file thousands of stabilization requests and track them throughout the day, all day, every day. That's what new versions are for, and it's already hard to keep up. Some critical fixes do have to be backported when a release is a complete dud, but it's just not feasible to backport every bugfix that can somehow affect sage. All of the time I and the other distribution developers spend pushing rocks up hills can't be spent on something more useful.

comment:22 follow-up: Changed 2 years ago by mkoeppe

Of course. With my comment above, I'm certainly not requesting any backports from you or the gentoo developers.

comment:23 in reply to: ↑ 22 Changed 2 years ago by mjo

Replying to mkoeppe:

Of course. With my comment above, I'm certainly not requesting any backports from you or the gentoo developers.

Intentionally or not, you're requesting backports from every distribution maintainer when you reject a system packages for lack of a bugfix.

comment:24 Changed 2 years ago by gh-kliem

  • Status changed from needs_info to needs_review

As it is, the ticket still appears to be working.

Mathematically our own doctests are correct with pari 2.11.2 and pari 2.11.4 and this ticket merely acknowledges this.

I think this ticket is still valuable, because now we can update pari to 2.11.4 without having to modify the doctests (and therefore leading to annoying failures with older versions). So the decision to update pari and to reject older version is then independent.

comment:25 Changed 2 years ago by dimpase

  • Reviewers set to Dima Pasechnik

ok, testing!

comment:26 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

this works well with system's pari 2.11.4 (as provided by Gentoo Linux).

comment:27 Changed 2 years ago by gh-kliem

Thank you.

comment:28 Changed 2 years ago by vbraun

  • Branch changed from public/29472 to 4f86bc37571a3c7912edb94d2976f857b93662b1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.