Opened 22 months ago
Closed 19 months ago
#29472 closed defect (fixed)
Modify doctest such that they work with pari >= 2.11.3 and pari < 2.11.3
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.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: 
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 debiansid https://github.com/kliem/sagetest27122/runs/560405581, which is (correctly) not fixed by #29445 (debian sid uses pari (2.11.4~pre11), which contains no serious bug that we are aware of).
Follow up: #29313.
Change History (28)
comment:1 Changed 22 months ago by
 Branch set to public/29472
 Commit set to ca4ee7d644ff3ee45b2c6e6c9ae8b5a08c54abda
comment:2 Changed 22 months ago by
 Commit changed from ca4ee7d644ff3ee45b2c6e6c9ae8b5a08c54abda to 98bfefda186e4c7992f7120b5956b5b41bfdcbb3
Branch pushed to git repo; I updated commit sha1. New commits:
98bfefd  small fix

comment:3 Changed 22 months ago by
 Status changed from new to needs_review
comment:4 Changed 22 months ago by
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/sagetest27122/actions/runs/72102779
comment:5 Changed 22 months ago by
 Cc mkoeppe mjo added
comment:6 followup: ↓ 8 Changed 22 months ago by
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 22 months ago by
 Commit changed from 98bfefda186e4c7992f7120b5956b5b41bfdcbb3 to 855098cb2f1c8d88592c2cb021bdb1bf099121fd
Branch pushed to git repo; I updated commit sha1. New commits:
855098c  create the context for the tests locally

comment:8 in reply to: ↑ 6 Changed 22 months ago by
I do this in many places, as in
A polyhedron:: sage: P = polytopes.cube() The fvector:: 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) TrueI thought that
::
cleared the globalK
. In any case, I would recommend reconstructingK
here so that the test doesn't break if someone adds another example to the end of theEXAMPLES:
block above it (and likewise for the other "test the above doctest" tests).
comment:9 Changed 22 months ago by
 Commit changed from 855098cb2f1c8d88592c2cb021bdb1bf099121fd to 4f86bc37571a3c7912edb94d2976f857b93662b1
Branch pushed to git repo; I updated commit sha1. New commits:
4f86bc3  fix failing doctest

comment:10 Changed 22 months ago by
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 followup: ↓ 12 Changed 22 months ago by
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 22 months ago by
Replying to ghkliem:
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 22 months ago by
ubuntu eoan has pari 2.11.22 and this seems to work perfectly fine with our doctests.
https://github.com/kliem/sagetest27122/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 anyQQ
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 22 months ago by
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 sagedevel. I'm demotivated 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 pari2.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 pari2.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 sidechannel for introducing version constraints.
comment:15 Changed 22 months ago by
I also think that a discussion on sagedevel  on the general topic of the relation of sagethedistribution 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 spkgconfigure 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 22 months ago by
 Milestone changed from sage9.1 to sage9.2
 Status changed from needs_review to needs_info
Ok, let's wait and see how we decide on things.
comment:17 Changed 21 months ago by
 Cc chapoton added
comment:18 followup: ↓ 19 Changed 19 months ago by
On this one it's probably not too muich work to make tests formattingindependent. 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 ; followup: ↓ 20 Changed 19 months ago by
Replying to dimpase:
On this one it's probably not too muich work to make tests formattingindependent. 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 dependencybranchrebase house of cards.
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 19 months ago by
Replying to mjo:
... essentially limiting support to the latest version ...
... or distribution's version with backported patches ...
comment:21 in reply to: ↑ 20 Changed 19 months ago by
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 followup: ↓ 23 Changed 19 months ago by
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 19 months ago by
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 19 months ago by
 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:26 Changed 19 months ago by
 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 19 months ago by
Thank you.
comment:28 Changed 19 months ago by
 Branch changed from public/29472 to 4f86bc37571a3c7912edb94d2976f857b93662b1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
modify doctests such that they work with various pari verions