failing doctest in sage/geometry/polyhedron/backend_normaliz.py (non-sorted)

[dkrenn]

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

[jdemeyer]

Duplicate of #23586

[dkrenn]

Replying to jdemeyer:

Duplicate of #23586

Thank you; I wasn't aware of this ticket.

[mderickx]

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.

[chapoton]

New commits:

​9b314dd

fixing pynormaliz doctests

[mderickx]

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.

[mderickx]

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.

[chapoton]

I agree that my branch is not a solution. Please write one.

[mderickx]

Ok, made the doctests deterministic

---

New commits:

​ce39e96

trac #23637 Make pynormaliz doctests deterministic

[tscrim]

PEP8: key = str) -> key=str). Otherwise LGTM and we can finally have these tests being deterministic.

[git]

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

​f117a5a

trac #23637 pep8 compatible space usage

[mderickx]

Ok fixed the formatting issues

[tscrim]

I think this is a good enough (temporary) solution.

[vbraun]

Merge conflict

[mderickx]

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.

[chapoton]

Be patient, You just need to wait for the next beta (end of the week ?).

[jdemeyer]

I'm fixing #23586.

[jdemeyer]

Why does this use sets anyway?

[jdemeyer]

New commits:

​18ac768

Don't use sets for no reason in doctests

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

[jdemeyer]

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

[mderickx]

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.

[mkoeppe]

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.

[jdemeyer]

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.

[mderickx]

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.

[tscrim]

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.

[jdemeyer]

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

[mkoeppe]

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.

[vbraun]

merge conflict

[tscrim]

Trivial rebase.

---

New commits:

​e6a0d3f

Merge branch 'u/jdemeyer/23637' of git://trac.sagemath.org/sage into u/tscrim/normaliz_test-23637