Opened 2 years ago

Closed 2 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) Commit: e6a0d3f7107dce115fb400c8eb2042168d7ac076
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

**********************************************************************
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: Changed 2 years ago by jdemeyer

  • Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #23586

comment:2 in reply to: ↑ 1 Changed 2 years ago by dkrenn

Replying to jdemeyer:

Duplicate of #23586

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

comment:3 Changed 2 years ago by mderickx

  • 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 2 years ago by chapoton

  • Branch set to u/chapoton/23586
  • Commit set to 9b314ddf075600df41a0433c5247abce218f36fb

New commits:

9b314ddfixing pynormaliz doctests

comment:5 Changed 2 years ago by 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.

comment:6 Changed 2 years ago by 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.

comment:7 Changed 2 years ago by chapoton

  • Branch u/chapoton/23586 deleted
  • Commit 9b314ddf075600df41a0433c5247abce218f36fb deleted

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

comment:8 Changed 2 years ago by mderickx

  • Branch set to u/mderickx/23637
  • Commit set to ce39e9619521a09215ce4eb876f78e4ac3939ab9
  • Status changed from new to needs_review

Ok, made the doctests deterministic


New commits:

ce39e96trac #23637 Make pynormaliz doctests deterministic

comment:9 Changed 2 years ago by tscrim

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

comment:10 Changed 2 years ago by git

  • Commit changed from ce39e9619521a09215ce4eb876f78e4ac3939ab9 to f117a5a9645477b655c13cc1c4c8a4a5632412ae

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

f117a5atrac #23637 pep8 compatible space usage

comment:11 Changed 2 years ago by mderickx

Ok fixed the formatting issues

comment:12 Changed 2 years ago by mderickx

  • Authors set to Maarten Derickx

comment:13 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:14 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:15 Changed 2 years ago by 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.

comment:16 Changed 2 years ago by chapoton

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

comment:17 Changed 2 years ago by jdemeyer

  • Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix

I'm fixing #23586.

comment:18 Changed 2 years ago by jdemeyer

Why does this use sets anyway?

comment:19 Changed 2 years ago by jdemeyer

  • Branch changed from u/mderickx/23637 to u/jdemeyer/23637

comment:20 Changed 2 years ago by jdemeyer

  • Authors changed from Maarten Derickx to Maarten Derickx, Jeroen Demeyer
  • 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:

18ac768Don't use sets for no reason in doctests

comment:21 follow-ups: Changed 2 years ago by tscrim

  • 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 2 years ago by 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...

comment:23 Changed 2 years ago by mderickx

  • 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: Changed 2 years ago by mkoeppe

  • 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 2 years ago by 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.

comment:26 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:27 follow-up: Changed 2 years ago by mderickx

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

Last edited 2 years ago by mderickx (previous) (diff)

comment:28 follow-up: Changed 2 years ago by 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.

comment:29 in reply to: ↑ 28 Changed 2 years ago by 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

comment:30 in reply to: ↑ 27 Changed 2 years ago by 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.

comment:31 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:32 Changed 2 years ago by tscrim

  • 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:

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

comment:33 Changed 2 years ago by vbraun

  • Branch changed from u/tscrim/normaliz_test-23637 to e6a0d3f7107dce115fb400c8eb2042168d7ac076
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.