Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#27965 closed enhancement (fixed)

py3: fix the last doctest in geometry

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.9
Component: python3 Keywords:
Cc: jipilab, vdelecroix, tscrim Merged in:
Authors: Frédéric Chapoton, John Palmieri Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 6fcdf05 (Commits) Commit: 6fcdf0594952b6aa3234a75ef0d7137c84655583
Dependencies: Stopgaps:

Description

or rather tag it with # py2

I have no idea why this does fail only in python3. But anyway, we cannot really do anything with the snub dodecahedron currently.

Change History (12)

comment:1 Changed 3 months ago by chapoton

  • Branch set to u/chapoton/27965
  • Cc tscrim added
  • Commit set to 7c9a636fa9198ccf3af036ec59ba459db5664416
  • Status changed from new to needs_review

very trivial ticket, please review swiftly


New commits:

7c9a636py3: fix the last doctest in geometry

comment:2 Changed 3 months ago by jhpalmieri

I think the issue here is not that polytopes.snub_dodecahedron() is broken in Python 3, but rather that Python 3 prints warning messages multiple times, whereas Python 2 only prints them once, at least while doctesting. A change like that in #27612 might be another option:

  • src/sage/geometry/polyhedron/library.py

    diff --git a/src/sage/geometry/polyhedron/library.py b/src/sage/geometry/polyhedron/library.py
    index 71914d635a..c9bb1f35e8 100644
    a b class Polytopes(): 
    14891489        Unfortunately, no polyhedra backend supports the construction of the
    14901490        snub dodecahedron at the moment::
    14911491
    1492             sage: sd = polytopes.snub_dodecahedron()
     1492            sage: sd = polytopes.snub_dodecahedron() # py2
     1493            sage: sd = polytopes.snub_dodecahedron() # py3
     1494            doctest:warning
     1495            ...
     1496            UserWarning: This polyhedron data is numerically complicated; cdd could not convert between the inexact V and H representation without loss of data. The resulting object might show inconsistencies.
    14931497            sage: sd.f_vector() # not tested
    14941498            (1, 60, 150, 92, 1)
    14951499            sage: sd.base_ring() # not tested

Given the comment, though ("no polyhedra backend supports the construction of the snub dodecahedron at the moment"), it's not a big deal.

Last edited 3 months ago by jhpalmieri (previous) (diff)

comment:3 Changed 3 months ago by jhpalmieri

  • Branch changed from u/chapoton/27965 to u/jhpalmieri/27965

comment:4 Changed 3 months ago by jhpalmieri

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, John Palmieri
  • Commit changed from 7c9a636fa9198ccf3af036ec59ba459db5664416 to 6fcdf0594952b6aa3234a75ef0d7137c84655583

I prefer my suggestion. When we move away from Python 2, it leaves a valid py3 command to execute.


New commits:

6fcdf05trac 27965: py3 fix in geometry/polyhedron/library.py

comment:5 Changed 3 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, good for me.

comment:6 Changed 3 months ago by jipilab

... have a look at #25097.

This test was removed and replaced by a reasonable test using the backend normaliz.

I would say that such computation over RDF should not be encouraged via examples/doctests as the backend normaliz provide reasonably fast computations with algebraic numbers.

comment:7 Changed 3 months ago by jipilab

cdd and python2 and 3 have different behavior. In #27760, I was obtaining several times this kind of warnings in the default case due to approximation errors. When such errors occurred, I simply change the default behavior _not_ to produce such warnings and go for an exact solution.

I would really try to simply avoid making such tests in the future as we know that they are prone to produce such errors and we have a viable faster and reliable alternative (ok, with an optional package...).

comment:8 Changed 3 months ago by chapoton

But #25097 is not yet approved. The main aim in the present ticket is to later be able to lock the geometry folder (which means adding it to the list of python3-known-passing folders), so that no py3 failure could happen by accident.

comment:9 follow-up: Changed 3 months ago by jhpalmieri

I think this can be merged and geometry white-listed, and then #25097 can replace the test here when that ticket is ready.

comment:10 in reply to: ↑ 9 Changed 3 months ago by jipilab

Replying to jhpalmieri:

I think this can be merged and geometry white-listed, and then #25097 can replace the test here when that ticket is ready.

All right, sounds good!

I put the present ticket as dependancy in #25097. The ticket is essentially ready and tested, it only gets slowed due to similar conflicts/dependancies, ;) that was the reason of my reserve.

comment:11 Changed 3 months ago by vbraun

  • Branch changed from u/jhpalmieri/27965 to 6fcdf0594952b6aa3234a75ef0d7137c84655583
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 3 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.

Note: See TracTickets for help on using tickets.