Opened 4 years ago
Closed 3 years ago
#23637 closed defect (fixed)
failing doctest in sage/geometry/polyhedron/backend_normaliz.py (non-sorted)
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.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.0-95-generic/arando/2017-08-17%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 follow-up: ↓ 2 Changed 4 years ago by
- Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix
- Resolution set to duplicate
- Status changed from new to closed
comment:2 in reply to: ↑ 1 Changed 4 years ago by
comment:3 Changed 4 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-8.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/sage-devel/_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 4 years ago by
- Branch set to u/chapoton/23586
- Commit set to 9b314ddf075600df41a0433c5247abce218f36fb
New commits:
9b314dd | fixing pynormaliz doctests
|
comment:5 Changed 4 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 4 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 4 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 4 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 4 years ago by
PEP8: key = str)
-> key=str)
. Otherwise LGTM and we can finally have these tests being deterministic.
comment:10 Changed 4 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 4 years ago by
Ok fixed the formatting issues
comment:12 Changed 4 years ago by
comment:13 Changed 4 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 4 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 4 years ago by
Be patient, You just need to wait for the next beta (end of the week ?).
comment:17 Changed 4 years ago by
- Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix
I'm fixing #23586.
comment:18 Changed 4 years ago by
Why does this use sets anyway?
comment:19 Changed 4 years ago by
- Branch changed from u/mderickx/23637 to u/jdemeyer/23637
comment:20 Changed 4 years ago by
- Commit changed from f117a5a9645477b655c13cc1c4c8a4a5632412ae to 18ac76860988348365df21196cef0da56cc6ab7f
- Component changed from doctest framework to doctest coverage
- Description modified (diff)
- Milestone changed from sage-duplicate/invalid/wontfix to sage-8.1
- Status changed from needs_work to needs_review
New commits:
18ac768 | Don't use sets for no reason in doctests
|
comment:21 follow-ups: ↓ 22 ↓ 24 Changed 4 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 4 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 4 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 ; follow-up: ↓ 25 Changed 4 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 4 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 4 years ago by
- Status changed from needs_work to needs_review
comment:27 follow-up: ↓ 30 Changed 4 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 --file-iterations=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 follow-up: ↓ 29 Changed 4 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 4 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 number-theoretical output
comment:30 in reply to: ↑ 27 Changed 4 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 3 years ago by
- Branch changed from u/jdemeyer/23637 to u/tscrim/normaliz_test-23637
- 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_test-23637
|
comment:33 Changed 3 years ago by
- Branch changed from u/tscrim/normaliz_test-23637 to e6a0d3f7107dce115fb400c8eb2042168d7ac076
- Resolution set to fixed
- Status changed from positive_review to closed
Duplicate of #23586