Opened 5 years ago
Closed 5 years ago
#23637 closed defect (fixed)
failing doctest in sage/geometry/polyhedron/backend_normaliz.py (nonsorted)
Reported by:  dkrenn  Owned by:  

Priority:  blocker  Milestone:  sage8.1 
Component:  doctest coverage  Keywords:  
Cc:  mkoeppe  Merged in:  
Authors:  Maarten Derickx, Jeroen Demeyer  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e6a0d3f (Commits, GitHub, GitLab)  Commit:  e6a0d3f7107dce115fb400c8eb2042168d7ac076 
Dependencies:  Stopgaps: 
Description (last modified by )
********************************************************************** File "src/sage/geometry/polyhedron/backend_normaliz.py", line 412, in sage.geometry.polyhedron.backend_normaliz.Polyhedron_normaliz.integral_hull Failed example: set(PI.Vrepresentation()) # optional  pynormaliz Expected: {A vertex at (1, 0), A vertex at (0, 1), A ray in the direction (1, 0)} Got: {A ray in the direction (1, 0), A vertex at (1, 0), A vertex at (0, 1)} ********************************************************************** File "src/sage/geometry/polyhedron/backend_normaliz.py", line 420, in sage.geometry.polyhedron.backend_normaliz.Polyhedron_normaliz.integral_hull Failed example: set(PI.Vrepresentation()) # optional  pynormaliz Expected: {A vertex at (1, 0), A ray in the direction (1, 0), A line in the direction (1, 1)} Got: {A line in the direction (1, 1), A ray in the direction (1, 0), A vertex at (1, 0)}
reported by patchbot https://patchbot.sagemath.org/log/23633/Ubuntu/14.04/i686/3.13.095generic/arando/20170817%2016:47:36
There is a simple solution: there is no reason at all to use set()
in these doctests, just get rid of that.
Change History (33)
comment:1 followup: ↓ 2 Changed 5 years ago by
 Milestone changed from sage8.1 to sageduplicate/invalid/wontfix
 Resolution set to duplicate
 Status changed from new to closed
comment:2 in reply to: ↑ 1 Changed 5 years ago by
comment:3 Changed 5 years ago by
 Milestone changed from sageduplicate/invalid/wontfix to sage8.1
 Priority changed from major to blocker
 Resolution duplicate deleted
 Status changed from closed to new
I'm reopening this ticket to server as ticket for solution 1 as described in https://groups.google.com/forum/#!topic/sagedevel/_2vtRbauvrw . I.e. just fix the doctest in pynormaliz itself and not rely on #23586 where the approach is making the doctest framework more robust, which might take longer then we want to wait for.
comment:4 Changed 5 years ago by
 Branch set to u/chapoton/23586
 Commit set to 9b314ddf075600df41a0433c5247abce218f36fb
New commits:
9b314dd  fixing pynormaliz doctests

comment:5 Changed 5 years ago by
I'm going to see if your branch is actually a solution and wether __cmp__
is deterministic. I suspect not and then this is not really a fix.
comment:6 Changed 5 years ago by
I just had a look, and as far as I can see it doesn't use __cmp__
but it has __ge__
and friends. And furthermore that these are not overwritten but all inherited from object , and hence not deterministic.
comment:7 Changed 5 years ago by
 Branch u/chapoton/23586 deleted
 Commit 9b314ddf075600df41a0433c5247abce218f36fb deleted
I agree that my branch is not a solution. Please write one.
comment:8 Changed 5 years ago by
 Branch set to u/mderickx/23637
 Commit set to ce39e9619521a09215ce4eb876f78e4ac3939ab9
 Status changed from new to needs_review
Ok, made the doctests deterministic
New commits:
ce39e96  trac #23637 Make pynormaliz doctests deterministic

comment:9 Changed 5 years ago by
PEP8: key = str)
> key=str)
. Otherwise LGTM and we can finally have these tests being deterministic.
comment:10 Changed 5 years ago by
 Commit changed from ce39e9619521a09215ce4eb876f78e4ac3939ab9 to f117a5a9645477b655c13cc1c4c8a4a5632412ae
Branch pushed to git repo; I updated commit sha1. New commits:
f117a5a  trac #23637 pep8 compatible space usage

comment:11 Changed 5 years ago by
Ok fixed the formatting issues
comment:12 Changed 5 years ago by
comment:13 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
I think this is a good enough (temporary) solution.
comment:15 Changed 5 years ago by
Hi Volker,
Could you be a bit more specific, which tickets cause the conflict. Or will there be a new beta4 soon on which I can rebase this. Because this message is not helping.
comment:16 Changed 5 years ago by
Be patient, You just need to wait for the next beta (end of the week ?).
comment:17 Changed 5 years ago by
 Milestone changed from sage8.1 to sageduplicate/invalid/wontfix
I'm fixing #23586.
comment:18 Changed 5 years ago by
Why does this use sets anyway?
comment:19 Changed 5 years ago by
 Branch changed from u/mderickx/23637 to u/jdemeyer/23637
comment:20 Changed 5 years ago by
 Commit changed from f117a5a9645477b655c13cc1c4c8a4a5632412ae to 18ac76860988348365df21196cef0da56cc6ab7f
 Component changed from doctest framework to doctest coverage
 Description modified (diff)
 Milestone changed from sageduplicate/invalid/wontfix to sage8.1
 Status changed from needs_work to needs_review
New commits:
18ac768  Don't use sets for no reason in doctests

comment:21 followups: ↓ 22 ↓ 24 Changed 5 years ago by
 Cc mkoeppe added
I believe it uses sets because there is no (natural) ordering on that output. However, I do not know if normaliz and our code has any guarantee on the output order. Maybe Matthias can comment more on this.
comment:22 in reply to: ↑ 21 Changed 5 years ago by
Replying to tscrim:
I believe it uses sets because there is no (natural) ordering on that output.
Well, clearly set()
doesn't solve that problem...
comment:23 Changed 5 years ago by
 Status changed from needs_review to needs_info
Yes we might not need set, but we do need know whether the output orderering albeit being arbitrary, is still deterministic at least. Because if it is not deterministic this still means we have flaky doctests and still need to sort.
comment:24 in reply to: ↑ 21 ; followup: ↓ 25 Changed 5 years ago by
 Status changed from needs_info to needs_work
Replying to tscrim:
I believe it uses sets because there is no (natural) ordering on that output. However, I do not know if normaliz and our code has any guarantee on the output order. Maybe Matthias can comment more on this.
Yes, the order of the output has no significance and to my understanding can change from version to version of normaliz.
I used set(...)
in the doctest, based on the misconception that this is the correct idiom. I now understand that I should have used sorted(...)
instead. I do not agree with the current patch, which just uses the unsorted output as is.
comment:25 in reply to: ↑ 24 Changed 5 years ago by
Replying to mkoeppe:
to my understanding can change from version to version of normaliz.
That's actually not bad at all. There are many doctests which depend on the version of a package. If the output is consistent for any given version, it is fine to put that output in the doctest.
comment:26 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:27 followup: ↓ 30 Changed 5 years ago by
 Status changed from needs_review to positive_review
I also just verified experimentally that the doctests are stable within a version by running:
./sage t fileiterations=100 src/sage/geometry/polyhedron/backend_normaliz.py
so I agree with Jeroen that his branch solves the main problem of this ticket, namely make the results of the patchbot reliable again. So positive review from me.
I understand that having doctests pass for multiple normaliz versions is something nice to have since it makes it easier to upgrade normaliz. But let's fix that problem for during the next upgrade of normaliz and not waste any more time on this ticket so that we can get consistent patchbot behaviour again.
comment:28 followup: ↓ 29 Changed 5 years ago by
Thank you for the explanation Matthias. However, I agree with Jeroen: the output is deterministic on the version of normaliz (which is bound to the Sage version by our current distribution methods, for better or worse), so directly comparing the output is good and clean as it does not require any extra modification to pass. Another package that we frequently have to update tests for is GAP.
comment:29 in reply to: ↑ 28 Changed 5 years ago by
Replying to tscrim:
Another package that we frequently have to update tests for is GAP.
...and PARI/GP.
The doctest framework does support # tol
for numerical differences but not # generates same group
or whatever for numbertheoretical output
comment:30 in reply to: ↑ 27 Changed 5 years ago by
Replying to mderickx:
I understand that having doctests pass for multiple normaliz versions is something nice to have since it makes it easier to upgrade normaliz. But let's fix that problem for during the next upgrade of normaliz and not waste any more time on this ticket so that we can get consistent patchbot behaviour again.
No objections here.
comment:32 Changed 5 years ago by
 Branch changed from u/jdemeyer/23637 to u/tscrim/normaliz_test23637
 Commit changed from 18ac76860988348365df21196cef0da56cc6ab7f to e6a0d3f7107dce115fb400c8eb2042168d7ac076
 Status changed from needs_work to positive_review
Trivial rebase.
New commits:
e6a0d3f  Merge branch 'u/jdemeyer/23637' of git://trac.sagemath.org/sage into u/tscrim/normaliz_test23637

comment:33 Changed 5 years ago by
 Branch changed from u/tscrim/normaliz_test23637 to e6a0d3f7107dce115fb400c8eb2042168d7ac076
 Resolution set to fixed
 Status changed from positive_review to closed
Duplicate of #23586