Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kliem opened this issue Apr 6, 2020 · 31 comments
Closed

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

kliem opened this issue Apr 6, 2020 · 31 comments

Comments

@kliem
Copy link
Contributor

kliem commented Apr 6, 2020

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.

CC: @mkoeppe @orlitzky @fchapoton

Component: packages: standard

Author: Jonathan Kliem

Branch/Commit: 4f86bc3

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/29472

@kliem kliem added this to the sage-9.1 milestone Apr 6, 2020
@kliem
Copy link
Contributor Author

kliem commented Apr 6, 2020

New commits:

ca4ee7dmodify doctests such that they work with various pari verions

@kliem
Copy link
Contributor Author

kliem commented Apr 6, 2020

Branch: public/29472

@kliem
Copy link
Contributor Author

kliem commented Apr 6, 2020

Commit: ca4ee7d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2020

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

98bfefdsmall fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2020

Changed commit from ca4ee7d to 98bfefd

@kliem
Copy link
Contributor Author

kliem commented Apr 7, 2020

comment:4

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

@orlitzky
Copy link
Contributor

comment:6

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2020

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

855098ccreate the context for the tests locally

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2020

Changed commit from 98bfefd to 855098c

@kliem
Copy link
Contributor Author

kliem commented Apr 10, 2020

comment:8

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 @orlitzky:

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2020

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

4f86bc3fix failing doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2020

Changed commit from 855098c to 4f86bc3

@orlitzky
Copy link
Contributor

comment:10

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.

@kliem
Copy link
Contributor Author

kliem commented Apr 12, 2020

comment:11

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 12, 2020

comment:12

Replying to @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.

@kliem
Copy link
Contributor Author

kliem commented Apr 12, 2020

comment:13

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.

@orlitzky
Copy link
Contributor

comment:14

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 12, 2020

comment:15

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.

@kliem
Copy link
Contributor Author

kliem commented Apr 14, 2020

comment:16

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

@kliem kliem modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@dimpase
Copy link
Member

dimpase commented Jun 18, 2020

comment:18

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.

@orlitzky
Copy link
Contributor

comment:19

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 19, 2020

comment:20

Replying to @orlitzky:

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

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

@orlitzky
Copy link
Contributor

comment:21

Replying to @mkoeppe:

Replying to @orlitzky:

... 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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 19, 2020

comment:22

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

@orlitzky
Copy link
Contributor

comment:23

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.

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

comment:24

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.

@dimpase
Copy link
Member

dimpase commented Jun 19, 2020

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jun 19, 2020

comment:25

ok, testing!

@dimpase
Copy link
Member

dimpase commented Jun 19, 2020

comment:26

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

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

comment:27

Thank you.

@vbraun
Copy link
Member

vbraun commented Jun 22, 2020

Changed branch from public/29472 to 4f86bc3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants