Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#22452 closed enhancement (fixed)

Create a Polymake pexpect interface

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-8.0
Component: packages: optional Keywords: Polymake, days84
Cc: vdelecroix, jipilab Merged in:
Authors: Simon King Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 52a77ee (Commits, GitHub, GitLab) Commit:
Dependencies: #22501 Stopgaps:

Status badges

Description

There is PyPolymake, to be added at #21170. But I think it wouldn't hurt to additionally have a pexpect interface to Polymake. And the purpose of this ticket is to create one.

Attachments (3)

22452-doctest.log (19.0 KB) - added by vdelecroix 4 years ago.
doctest failure with polymake beta
doctest-22452-polymake3.0.6.log (9.7 KB) - added by vdelecroix 4 years ago.
doctest at commit afdbcdb for polymake 3.0.6
doctest-22452-polymake3.0.9.log (11.6 KB) - added by vdelecroix 4 years ago.
doctest at commit afdbcdb for polymake 3.0.9

Download all attachments as: .zip

Change History (105)

comment:1 Changed 4 years ago by SimonKing

  • Keywords days84 added; days85 removed

comment:2 Changed 4 years ago by SimonKing

  • Branch set to u/SimonKing/create_a_polymake_pexpect_interface

comment:3 Changed 4 years ago by vdelecroix

  • Cc vdelecroix added
  • Commit set to c3add962d2d518164765993b70e843d10f17a4da

Last 10 new commits:

33b53f3Add documentation to _get_primitive
d78b239'Implement string representation by single underscore methods': Apply this mantra to interfaces
9a0bccaChange string representation of invalid interface elements whose parent is None
6910107Keep the tests that have been removed in the previous commits
cd97ff3Parse error message of _check_valid() in order to cope with unpredictable future semantical changes
fdaeb2eInclude error message of validity check into string representation of invalid interface elements
cdb085bRevert introduction of _get_custom_name
b5df135Actually remove _get_custom_name, not only its applications
6b53229Reverse addition of _get_primitive. Fix __cmp__, bool etc. and their documentation
c3add96Initial wrapper: providing help, tab completion, accessing members, calling functions, nice(r) string representation

comment:4 Changed 4 years ago by SimonKing

Here I am showcasing some things that work already:

sage: from sage.interfaces.polymake import polymake
sage: polymake.help("objects/Cone/properties/Combinatorics/RAYS_IN_FACETS")
property RAYS_IN_FACETS : IncidenceMatrix<NonSymmetric>
 Ray-facet incidence matrix, with rows corresponding to facets and columns
 to rays. Rays and facets are numbered from 0 to N_RAYS-1 rsp.
 N_FACETS-1, according to their order in RAYS rsp. FACETS.

sage: polymake.help('rand_sphere')
functions/Producing a polytope from scratch/rand_sphere:
rand_sphere(d, n; Options) -> Polytope

 Produce a d-dimensional polytope with n random vertices
 uniformly distributed on the unit sphere.

Arguments:
  Int d the dimension
  Int n the number of random vertices

Options:
  seed => Int controls the outcome of the random number generator;
    fixing a seed number guarantees the same outcome. 

Returns Polytope 

So, there is access to Polymake' help system.

sage: P = polymake.rand<tab key>
                polymake.rand01                polymake.rand_knot              
                polymake.rand_aof              polymake.rand_metric            
                polymake.rand_box              polymake.rand_metric_int       >
                polymake.rand_cyclic           polymake.rand_perm              
                polymake.rand_inner_points     polymake.rand_seed 

So, tab completion of the interface itself works, it yields ALL function names, including those that are not part of the current Polymake application.

sage: P = polymake.rand_sphere(3,10,seed=5)

So, calling a function with parameters works

sage: P
Random spherical polytope of dimension 3; seed=5

So, string representation works, actually nicer than in Polymake (more examples below).

sage: P.known_properties()   # members computed so far
['BOUNDED', 'CONE_AMBIENT_DIM', 'FEASIBLE', 'POINTS']
sage: P.get_schedule('F_VECTOR')

precondition : BOUNDED ( lrs.convex_hull.count: N_FACETS : RAYS | INPUT_RAYS )
lrs.convex_hull.count: N_FACETS : RAYS | INPUT_RAYS
sensitivity check for VertexPerm
cdd.convex_hull.canon: POINTED, RAYS, LINEALITY_SPACE : INPUT_RAYS
CONE_DIM : RAYS | INPUT_RAYS
N_RAYS : RAYS
precondition : POINTED ( LINEALITY_DIM, LINEALITY_SPACE : )
LINEALITY_DIM, LINEALITY_SPACE :
COMBINATORIAL_DIM : CONE_DIM, LINEALITY_DIM
precondition : COMBINATORIAL_DIM ( F_VECTOR : N_FACETS, N_RAYS, COMBINATORIAL_DIM )
F_VECTOR : N_FACETS, N_RAYS, COMBINATORIAL_DIM

So, this shows how the f-vector would be computed for this polytope.

sage: V = P.VER<tab key>
                P.VERTEX_BARYCENTER        P.VERTICES                 
                P.VERTEX_LABELS            P.VERTICES_IN_FACETS       
                P.VERTEX_NORMALS           P.VERTICES_IN_INEQUALITIES 
                P.VERTEX_SIZES             P.VERY_AMPLE               

So, tab completion for Polymake elements works. It shows both members (including those that aren't computed yet) and functions (including those that won't be applicable to the object).

sage: set_verbose(3)
sage: V = P.VERTICES
sage: F = P.F_VECTOR
polymake: used package lrs
  Implementation of the reverse search algorithm of Avis and Fukuda.
  Copyright by David Avis.
  http://cgm.cs.mcgill.ca/~avis/lrs.html

So, when verbosity is set to a positive value, comments printed by Polymake will be printed by Sage.

sage: F
10 24 16
sage: V

1 -549467720344047/2251799813685248 -1666038093248221/2251799813685248 5646952731601237/9007199254740992
1 396443771972991/562949953421312 -3761317241461329/36028797018963968 6325385137097525/9007199254740992
1 8731416515056451/9007199254740992 8225904699783425/36028797018963968 6513541817898829/72057594037927936
1 -6649740931032245/9007199254740992 -316039202166671/562949953421312 -6735311418008155/18014398509481984
1 1027872253618593/1125899906842624 -4941443956760209/18014398509481984 -5443456128447233/18014398509481984
1 3237778007687205/4503599627370496 -3127563668227821/4503599627370496 8478171037802839/288230376151711744
1 4654944693569921/9007199254740992 -2595293938705027/4503599627370496 5702536786492105/9007199254740992
1 -4153090305917205/4503599627370496 2814681273371419/18014398509481984 -3186909849764487/9007199254740992
1 6185041161130579/9007199254740992 -2775723053730255/4503599627370496 -6944451127537363/18014398509481984
1 -6106052881221159/18014398509481984 -2063008243044317/18014398509481984 -8410984913482021/9007199254740992

So, string representation is (at least for the things that are currently wrapped) meaningful.

sage: F[2]
16
sage: V[3][3]
-6735311418008155/18014398509481984

So, attribute access works.

sage: V.foobar()
Traceback (most recent call last)
...
TypeError: polymake:  ERROR: Undefined subroutine &Polymake::User::foobar called at input line 1.
sage: polymake.eval('foobar(4)')
^CInterrupting Polymake...
Traceback (most recent call last)
...
KeyboardInterrupt: Ctrl-c pressed while running Polymake
sage: polymake.eval('foobar(4);')
Traceback (most recent call last)
...
PolymakeError: polymake:  ERROR: Undefined subroutine &Polymake::User::foobar called at input line 1.

So, error handling is implemented.

Meanwhile, more members are known:

sage: P.known_properties()

['AFFINE_HULL',
 'BOUNDED',
 'COMBINATORIAL_DIM',
 'CONE_AMBIENT_DIM',
 'CONE_DIM',
 'FAR_FACE',
 'FEASIBLE',
 'FULL_DIM',
 'F_VECTOR',
 'LINEALITY_DIM',
 'LINEALITY_SPACE',
 'N_FACETS',
 'N_POINTS',
 'N_VERTICES',
 'POINTED',
 'POINTS',
 'VERTICES']

Of course, some things won't work yet. For example, I am not catching warnings yet.

Last edited 4 years ago by SimonKing (previous) (diff)

comment:5 Changed 4 years ago by git

  • Commit changed from c3add962d2d518164765993b70e843d10f17a4da to bfae4863bf12ec555ce7976fc39ce42a16a12fcc

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

bfae486Implement getting length of arrays and hashes

comment:6 Changed 4 years ago by SimonKing

Example for the next commit:

sage: from sage.interfaces.polymake import polymake
sage: P = polymake.rand_sphere(3,10,seed=5)
sage: len(P)   # A polytope is in fact internally represented by an array
14
sage: V = P.VERTICES
sage: len(V)
10
sage: len(V[2])
4

New commits:

bfae486Implement getting length of arrays and hashes

comment:7 Changed 4 years ago by jipilab

  • Cc jipilab added

comment:8 Changed 4 years ago by SimonKing

Now I am totally puzzled. After doing some changes, many thinks didn't work. Thus, I reverted all changes. I even deleted my local git branch and downloaded the branch from here. But things still don't work (tab completion etc). Of course I did sage -br.

Do you have any idea what could cause such persisting problems?

comment:9 follow-up: Changed 4 years ago by SimonKing

It seems that my Sage installation is totally broken. I did "make start" and it seems that it installs python3 without my asking for it.

comment:10 Changed 4 years ago by git

  • Commit changed from bfae4863bf12ec555ce7976fc39ce42a16a12fcc to c3e7d475d39678c941160bedf9033db6fd7e9c85

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

c3e7d47Fix reading from file. Fix KeyboardInterrupt for Polymake

comment:11 Changed 4 years ago by git

  • Commit changed from c3e7d475d39678c941160bedf9033db6fd7e9c85 to 2bd2ac5d3b5931acdfdec516e16a875356b7c646

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

2bd2ac5add synchronisation code for Polymake interface

comment:12 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by vdelecroix

Replying to SimonKing:

It seems that my Sage installation is totally broken. I did "make start" and it seems that it installs python3 without my asking for it.

Yes. That is normal. python2 and python3 do install by default now (since 2 or 3 beta already).

comment:13 in reply to: ↑ 12 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Replying to SimonKing:

It seems that my Sage installation is totally broken. I did "make start" and it seems that it installs python3 without my asking for it.

Yes. That is normal. python2 and python3 do install by default now (since 2 or 3 beta already).

Thanks, I didn't notice that before.

I still don't know what went wrong, but it is now again working, including the synchronisation code that I posted in the previous commit.

comment:14 Changed 4 years ago by git

  • Commit changed from 2bd2ac5d3b5931acdfdec516e16a875356b7c646 to 06e85fd9d84b15f06c4aa2e14d4db84c009c0459

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

06e85fdKeep polymake interface in sync. Take docstring from polymake. Fix calling of polymake functions

comment:15 Changed 4 years ago by SimonKing

Finally I found a reasonable source explaining a part of perl that seems particularly relevant: http://perldoc.perl.org/perlref.html

So, I am now rewriting a couple of things in terms of what I learn about perl references.

comment:16 Changed 4 years ago by SimonKing

It is of course conceivable that I am just lacking experience, but I find Perl sucks.

In order to write an interface, it is important to do things uniformly. Hence, all variables created by Sage in Polymake should be "the same". In the current branch, "the same" means that they are all references (which in Perl means they are SCALAR).

So, how to assign any object x in Polymake to a variable var?

If you define a SCALAR "var" and assign to "x", you of course do not get a reference to x but some scalar interpretation of whatever is in x. Thus, I need to create a reference to x. Sounds trivial (and perhaps it is). However, all what I found is that one needs to do $var=\@x, $var=\%x or $var=\*x depending on whether x is an array, a hash or an io stream. OK, sounds easy. But so far I found no way to make Perl tell me what x really is.

Instead, let var uniformly be an array whose single entry is x. Thus:

polytope > @P = [rand_sphere(4,33, seed=>5)];

polytope > print @P[0];

polymake:  ERROR: Scalar value @Polymake::User::P[0] better written as $Polymake::User::P[0] at input line 1.

WTF?? I should better replace something that I did not write??? From what I thought I have learnt about Perl syntax, @P[0] should give the first entry in the array P!

Last edited 4 years ago by SimonKing (previous) (diff)

comment:17 Changed 4 years ago by SimonKing

Aha, my mistake. Apparently it works slightly differently:

polytope > @P = (rand_sphere(4,33, seed=>5));
polytope > print $P[0];
Polymake::polytope::Polytope__Rational=ARRAY(0x80d0370)
polytope > print $P[0][1];
Random spherical polytope of dimension 4; seed=5

So, creating a new variable that points to the content of the old variable should work like this, and it does:

polytope > @Q = ($P[0]);

polytope > print $Q[0];
Polymake::polytope::Polytope__Rational=ARRAY(0x80d0370)
polytope > print $Q[0][1];
Random spherical polytope of dimension 4; seed=5

Access to members also works as expected:

polytope > @V = ($Q[0]->VERTICES);
...
polytope > print $V[0];
...
1 -2185542006599419/2251799813685248 -5220912692532935/36028797018963968 2637694986535851/72057594037927936 3401062251409073/18014398509481984
polytope > @S = ($Q[0]->get_schedule("H_VECTOR"));
polymake: used package ppl
  The Parma Polyhedra Library (PPL): A C++ library for convex polyhedra
  and other numerical abstractions.
  http://www.cs.unipr.it/ppl/


polytope > print join(", ", $S[0]->list);
precondition : N_RAYS | N_INPUT_RAYS ( ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS ), sensitivity check for FacetPerm, ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS, RAYS_IN_FACETS : RAYS, FACETS, GRAPH.ADJACENCY : RAYS_IN_FACETS, DUAL_GRAPH.ADJACENCY : RAYS_IN_FACETS, SIMPLICIAL : COMBINATORIAL_DIM, RAYS_IN_FACETS, N_EDGES : ADJACENCY ( applied to DUAL_GRAPH ), N_EDGES : ADJACENCY ( applied to GRAPH ), N_RAYS : RAYS, N_FACETS : FACETS, precondition : COMBINATORIAL_DIM ( F_VECTOR : N_FACETS, N_RAYS, GRAPH.N_EDGES, DUAL_GRAPH.N_EDGES, COMBINATORIAL_DIM ), F_VECTOR : N_FACETS, N_RAYS, GRAPH.N_EDGES, DUAL_GRAPH.N_EDGES, COMBINATORIAL_DIM, precondition : SIMPLICIAL ( H_VECTOR : F_VECTOR ), H_VECTOR : F_VECTOR

So, I'll rewrite my branch accordingly.

comment:18 Changed 4 years ago by SimonKing

Argh. The same trick won't work when I want a list in a list:

polytope > print join(", ", $L[0]);
precondition : N_RAYS | N_INPUT_RAYS ( ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS )

comment:19 Changed 4 years ago by SimonKing

Next thing to try: Use a reference to an array. According to http://perldoc.perl.org/5.20.0/perllol.html, I expect it to work as follows:

polytope > $P = [rand_sphere(4,33, seed=>5)];
polytope > print $P->[0];
Polymake::polytope::Polytope__Rational=ARRAY(0x7c9bae0)
polytope > print $P->[0][1];
Random spherical polytope of dimension 4; seed=5
polytope > $Q = [$P->[0]];
polytope > print $Q->[0];
Polymake::polytope::Polytope__Rational=ARRAY(0x7c9bae0)
polytope > print $Q->[0][1];
Random spherical polytope of dimension 4; seed=5
polytope > $V = [$Q->[0]->VERTICES];
polytope > print $V->[0];
...
polytope > $S = [$Q->[0]->get_schedule("H_VECTOR")];
polytope > print join(", ", $S->[0]->list);
precondition : N_RAYS | N_INPUT_RAYS ( ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS ), sensitivity check for FacetPerm, ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS, RAYS_IN_FACETS : RAYS, FACETS, GRAPH.ADJACENCY : RAYS_IN_FACETS, DUAL_GRAPH.ADJACENCY : RAYS_IN_FACETS, SIMPLICIAL : COMBINATORIAL_DIM, RAYS_IN_FACETS, N_EDGES : ADJACENCY ( applied to DUAL_GRAPH ), N_EDGES : ADJACENCY ( applied to GRAPH ), N_RAYS : RAYS, N_FACETS : FACETS, precondition : COMBINATORIAL_DIM ( F_VECTOR : N_FACETS, N_RAYS, GRAPH.N_EDGES, DUAL_GRAPH.N_EDGES, COMBINATORIAL_DIM ), F_VECTOR : N_FACETS, N_RAYS, GRAPH.N_EDGES, DUAL_GRAPH.N_EDGES, COMBINATORIAL_DIM, precondition : SIMPLICIAL ( H_VECTOR : F_VECTOR ), H_VECTOR : F_VECTOR
polytope > print join(", ", $L->[0]);
precondition : N_RAYS | N_INPUT_RAYS ( ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS )

So, for wrapping lists, it won't work either.

Version 0, edited 4 years ago by SimonKing (next)

comment:20 follow-up: Changed 4 years ago by SimonKing

It seems that the problem with nested arrays is known and is discussed here.

comment:21 in reply to: ↑ 20 Changed 4 years ago by SimonKing

Apparently there are no arrays of arrays in Perl. Instead, each item in an array has to be a scalar. Hence, an array of arrays would in fact be implemented as an array of references to arrays.

And it seems that Perl would automagically create such references if you assign something to an entry of an existing array, according to the reference given in the previous post. So, I expect that things would finally work as follows:

polytope > $P = []; $P->[0] = rand_sphere(4,33,seed=>5);
polytope > print $P->[0][1];
Random spherical polytope of dimension 4; seed=5
polytope > $Q = []; $Q->[0] = $P->[0];
polytope > print $Q->[0][1];
Random spherical polytope of dimension 4; seed=5
polytope > $V = []; $V->[0] = $Q->[0]->VERTICES;
polytope > print $V->[0];
...  # output as expected
polytope > $S = []; $S->[0] = $Q->[0]->get_schedule("H_VECTOR");
polytope > print join("\n ", $S->[0]->list);
precondition : N_RAYS | N_INPUT_RAYS ( ppl.convex_hull.primal: FACETS, LINEAR_SPAN : RAYS | INPUT_RAYS )
 sensitivity check for FacetPerm
...   # output as expected
polytope > $L = []; $L->[0] = $S->[0]->list;

polytope > print join("\n ", $L->[0]);
15

WTF??

I suppose the problem here is that in the line $L->[0] = $S->[0]->list; perl puts a scalar value into the first entry of the list that L points to. In contrast to what I thought perl would do, it does not put a reference (which is a scalar) to $S->[0]->list into the first entry of L, but it puts there a scalar interpretation of the array $S->[0]->list --- which is the length of that array.

Crap.

comment:22 Changed 4 years ago by SimonKing

I will now try this model:

  • A variable created by Sage in the interface are arrays.
  • Such variable either corresponds to a genuine Perl array, or it is an array of length one whose single entry is a reference to some Polymake object.
  • I only need to distinguish one thing: If var is an array of length>0, then it is guaranteed to be a genuine Perl array, and we consistently use var as @var. Otherwise, we use it in the form $var[0].
  • Since the length of the array is determined at the time of its creation, I can store an attribute in the Sage wrapper that tells what case we'll have to deal with.
  • As far as I can see, the only problem would be a genuine Perl array x of length 1, that we would mistakenly interpret as a Polymake object wrapped in x; this would only be problematic if the array x would later be modified.

Anyway. I will next try to use this approach to assign expr in Polymake to a variable:

  • @SAGE0 = (expr);, and "SAGE0" is the autogenerated name of a Sage wrapper for expr.
  • Test the length of @SAGE0. If the length is 1, then the sage wrapper stores the information that to access the wrapped value it has to use $SAGE0[0]. Otherwise, it stores the information that to access the wrapped value it has to use @SAGE0.
  • Do not care about the case that expr is an array of length one.

comment:23 Changed 4 years ago by git

  • Commit changed from 06e85fd9d84b15f06c4aa2e14d4db84c009c0459 to 69e01f35fc42e27b2b6f3b01357c84ff3fb0a2ad

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

e801cabChange to a different approach to wrapping Perl variables
87ad586Change syntax for catching 'ValueError as...'
fec57d1Merge branch 't/22501/make_it_easier_to_customize_pexpect_interfaces' into t/22452/create_a_polymake_pexpect_interface
69e01f3Small change to how certain element types are printed

comment:24 Changed 4 years ago by SimonKing

I have implemented the approach explained in the previous comment.

Here I show that I can basically recreate a rather non-trivial computation from this polymake tutorial:

sage: from sage.interfaces.polymake import polymake
sage: p = polymake.cube(3)
sage: set_verbose(3)
sage: p.N_LATTICE_POINTS
used package latte
  LattE (Lattice point Enumeration) is a computer software dedicated to the 
  problems of counting lattice points and integration inside convex polytopes.
  Copyright by Matthias Koeppe, Jesus A. De Loera and others.
  http://www.math.ucdavis.edu/~latte/
27
sage: p.HILBERT_BASIS_GENERATORS
used package 4ti2
  4ti2 -- A software package for algebraic, geometric and combinatorial problems on linear spaces.
  Copyright by 4ti2 team.
  http://www.4ti2.de/

<1 0 -1 -1
1 1 -1 -1
1 -1 1 -1
1 1 1 1
1 -1 -1 -1
1 0 0 -1
1 1 0 -1
1 -1 0 -1
1 0 1 -1
1 1 1 -1
1 0 -1 0
1 1 -1 0
1 -1 1 0
1 -1 -1 0
1 0 0 0
1 1 0 0
1 -1 0 0
1 0 1 0
1 1 1 0
1 0 -1 1
1 1 -1 1
1 -1 1 1
1 -1 -1 1
1 0 0 1
1 1 0 1
1 -1 0 1
1 0 1 1
>
<>

I am not totally happy with the string representation of this, but I guess it is still nice enough. Note that a different package is used compared with what is stated in the tutorial. Probably this is because of a different polymake version:

sage: set_verbose(0)
sage: polymake.version()
'3.0'

whereas the tutorial uses version 2.9.6.

With the pexpect interface, it is also possible to create a new instance of a Polymake class, but the typical name ("new") is not available, as a method with this name but with a different semantics exists in Sage's interface framework. So, I chose a different name:

sage: q = polymake.new_from_type("Polytope", INEQUALITIES=[[5,-4,0,1],[-3,0,-4,1],[-2,1,0,0],[-4,4,4,-1],[0,0,1,0],[8,0,0,-1],[1,0,-1,0],[3,-1,0,0]])
sage: q
Polytope<Rational>[SAGE45]

Again, the string representation could be subject of discussion. The current proposal is to take into account the type name and the name of the Sage variable representing the instance.

sage: set_verbose(3)
sage: q.VERTICES
used package ppl
  The Parma Polyhedra Library (PPL): A C++ library for convex polyhedra
  and other numerical abstractions.
  http://www.cs.unipr.it/ppl/

1 3 0 8
1 2 1 8
1 3 1 8
1 2 0 3
1 3 0 7
1 3 1 7
1 2 1 7
1 2 0 4
sage: q.NORMAL

sage: q.HILBERT_BASIS_GENERATORS

<1 3 0 8
2 5 1 13
1 2 1 8
1 2 0 3
1 3 0 7
1 2 1 7
1 3 1 7
1 3 1 8
1 2 0 4
>
<>
sage: q.VERY_AMPLE
1

I think the order of facets has changed between versions, since some of the following differs from what is in the tutorial:

sage: f = q.facet(2)
sage: f.VERY_AMPLE
1
sage: f.VERTICES

1 2 1 8
1 2 0 3
1 2 1 7
1 2 0 4
sage: q.FACETS

5 -4 0 1
-3 0 -4 1
-2 1 0 0
-4 4 4 -1
0 0 1 0
8 0 0 -1
1 0 -1 0
3 -1 0 0

Here I show that Polymake's warnings are taken care of (and so are errors):

sage: f.FACET_WIDTH
/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/interfaces/polymake.py:464: RuntimeWarning: could not compute 'FACET_WIDTH' probably because of unsatisfied preconditions:
precondition : FULL_DIM ( FACET_WIDTHS : VERTICES , FACETS , CONE_AMBIENT_DIM )
  warnings.warn(w, RuntimeWarning)

sage: g = f.ambient_lattice_normalization()
sage: q.FACET_WIDTH
5
sage: cr = p.vertex_lattice_normalization()
sage: cr.VERTICES

(4) (0 1)
1 1 0 0
1 0 1 0
1 1 1 0
1 0 0 1
1 1 0 1
1 0 1 1
1 1 1 1

In polymake, you sometimes need to load a different application in order to work. In terms of the pexpect interface, this is a non-trivial change, as the current prompt depends on the current application. Nonetheless, I made it work:

sage: polymake.application('fan')
sage: f = p.normal_fan()
sage: f.SMOOTH_FAN
1
sage: g = q.normal_fan()
sage: g.RAYS

-1 0 1/4
0 -1 1/4
1 0 0
1 1 -1/4
0 1 0
0 0 -1
0 -1 0
-1 0 0
sage: g.RAYS.primitive()

-4 0 1
0 -4 1
1 0 0
4 4 -1
0 1 0
0 0 -1
0 -1 0
-1 0 0
sage: g.MAXIMAL_CONES

{3 4 5 7}
{2 3 5 6}
{5 6 7}
{0 1 2 4}
{0 4 7}
{0 1 6 7}
{1 2 6}
{2 3 4}
sage: g.HASSE_DIAGRAM.FACES.rows_numbered()

0:
1:0
2:1
3:2
4:3
5:4
6:5
7:6
8:7
9:0 1
10:0 4
11:0 7
12:1 2
13:1 6
14:2 3
15:2 4
16:2 6
17:3 4
18:3 5
19:4 7
20:5 6
21:5 7
22:6 7
23:3 4 5 7
24:2 3 5 6
25:5 6 7
26:0 1 2 4
27:0 4 7
28:0 1 6 7
29:1 2 6
30:2 3 4
31:0 1 2 3 4 5 6 7
sage: g.HASSE_DIAGRAM.nodes_of_dim(-2)
{9 10 11 12 13 14 15 16 17 18 19 20 21 22}

Visualisation does not work. I don't know if that can be achieved with a pexpect interface.

TODO_

Add documentation. Apart from that, I currently see no obvious issue.


New commits:

e801cabChange to a different approach to wrapping Perl variables
87ad586Change syntax for catching 'ValueError as...'
fec57d1Merge branch 't/22501/make_it_easier_to_customize_pexpect_interfaces' into t/22452/create_a_polymake_pexpect_interface
69e01f3Small change to how certain element types are printed

comment:25 Changed 4 years ago by git

  • Commit changed from 69e01f35fc42e27b2b6f3b01357c84ff3fb0a2ad to 054123b0e98db7040fa83682283130e4fff62dc9

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

054123bExtending a method to use 'new' for Polymake classes

comment:26 follow-up: Changed 4 years ago by SimonKing

I think I will change the strategy for wrapping yet again.

Basic problem to solve

In Perl, the different basic types HASH, ARRAY, SCALAR, and IO require a different syntax in order to perform "equivalent" operations on them. Even to assign some data X to a variable, the required syntax depends on the type of X. So, if I don't know in advance the type of some data X then there's your problem.

My current approach is to store all data in an ARRAY: If the data X is in fact an ARRAY then the wrap is the same as X, as Perl doesn't know arrays of arrays; otherwise the wrap is a list of length one containing X. So, assigning data X to a wrapper variable W, I know how to do so independent of the type of X. For all subsequent operations on X, I can test the type of the first entry of W (if len(W)==1) respectively I know that X is the same as W, and then know what to do.

Caveat: My current approach would fail if X happens to be an array of length 1.

I think a cleaner solution would be this: Write a little Perl function get_type_of, such that get_type_of(X) returns the array ("ARRAY", X) resp. ("HASH", X) resp. ("SCALAR", X) depending on the type of X --- this could probably be done by some "try - catch" magic.

Now, when wrapping a polymake object in the pexpect interface, the output of get_type_of would be used to determine whether we need to evaluate $SAGE1 = X; or @SAGE1 = X; or %SAGE1 = X; to create the wrapper. Of course, the type would be stored, so that in all later operations it is clear whether, for example, item access is done be @SAGE1[14] or %SAGE1{14}.

But this has to wait for later, as I should be busy with different things today.

comment:27 Changed 4 years ago by git

  • Commit changed from 054123b0e98db7040fa83682283130e4fff62dc9 to 15b6cc65c7415656df8881214ea6eb4720e6dc74

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

15b6cc6Remove automated creation of a logfile

comment:28 Changed 4 years ago by git

  • Commit changed from 15b6cc65c7415656df8881214ea6eb4720e6dc74 to b9009929e066bf5c002836702ab968bbc098ba9d

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

b900992fix a bug in getting docs from polymake

comment:29 in reply to: ↑ 26 Changed 4 years ago by SimonKing

Replying to SimonKing:

I think I will change the strategy for wrapping yet again. ... I think a cleaner solution would be this: Write a little Perl function get_type_of, such that get_type_of(X) returns the array ("ARRAY", X) resp. ("HASH", X) resp. ("SCALAR", X) depending on the type of X --- this could probably be done by some "try - catch" magic.

I tried, and it seems it won't work. At least it won't work better than the current approach. So, I'll keep it.

Andreas Paffenholz has just pointed out to me how to parse the output of apropos ""; in order to not only get the functions of the current application, but also to get member and member function names, which is important for tab completion respectively for knowing what a Sage user means when she types X.name_of_something: An property of X? A polymake function that, when called, is applied to X and further given arguments? A member function of X that, when called, is evaluated only on the further given arguments?

Also I made some experiments with KeyboardInterrupt. Sometimes I found that it crashed the interface (i.e., polymake._expect became None), and so far I don't understand why this can happen, and why it doesn't always happen.

Last edited 4 years ago by SimonKing (previous) (diff)

comment:30 Changed 4 years ago by SimonKing

  • Dependencies set to 22501

comment:31 Changed 4 years ago by SimonKing

  • Dependencies changed from 22501 to #22501

comment:32 Changed 4 years ago by SimonKing

In addition to the problem with keyboard interrupt, I got this problem (probably related):

sage: from sage.interfaces.polymake import polymake
sage: p = polymake.rand_sphere(4,23, seed=5)
sage: t = p.TRIANGULATION
sage: t?
Traceback (most recent call last):
...
RuntimeError: Polymake terminated unexpectedly while reading in a large line:
unknown help topic 'Polymake::topaz::GeometricSimplicialComplex__Rational::__as__Polymake::polytope::Polytope::_prop_TRIANGULATION'

Andreas and Julian have told me a bit more about several kinds of type names, and it seems they would be useful both to access the documentation and to get a more accurate list of available properties and member functions.

comment:33 Changed 4 years ago by SimonKing

A new complication arose: So far, the patterns I was looking for with pexpect have been application_name > (normal prompt), application_name (digits) > (that's the continuation prompt), killed by signal, polymake: ERROR:, and polymake: WARNING.

But I have just learned that when asking for help "Triangulation" there is yet another prompt to take care of:

polytope > help "TRIANGULATION";
There are 5 help topics matching 'TRIANGULATION':
1: objects/Cone/properties/Triangulation and volume/TRIANGULATION
2: objects/Polytope/properties/Triangulation and volume/TRIANGULATION
3: objects/Visualization/Visual::PointConfiguration/methods/TRIANGULATION
4: objects/Visualization/Visual::Polytope/methods/TRIANGULATION
5: objects/PointConfiguration/properties/Triangulation and volume/TRIANGULATION
-------------------
Please choose those interesting you via history navigation (ArrowUp/ArrowDown):

polytope [1]> 

I was told by the polymake developers at !SageDays84 that the prompt with square brackets will only arise when requesting help.

So, what shall we do in such situation? Display the available information (using _sage_doc_) and leave it to the user to access the different help topics? Always choose the first available help option? Other ideas?

comment:34 Changed 4 years ago by SimonKing

I guess the easiest would be to generally look for the application_name [digits] > pattern and send a chr(3) when it occurs, as this should bring back the normal prompt. And then return the output on top of the pattern. So,that would be the solution "leave it to the user to access the different help topics". And _sage_doc_ could in that case simply return the empty string (or maybe the string representation of the Polymake type).

comment:35 Changed 4 years ago by git

  • Commit changed from b9009929e066bf5c002836702ab968bbc098ba9d to 3e5b32df91d72075ee5470c22282ca93c828abf9

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

d9e2b6fUse a more recommended way to get a description of a big object in Polymake
63418a6Fix that when requesting help, Polymake may request user interrupt.
54ea91cMerge branch 'develop' into t/22452/create_a_polymake_pexpect_interface
3e5b32dAdd some documentation for the Polymake interface

comment:36 Changed 4 years ago by git

  • Commit changed from 3e5b32df91d72075ee5470c22282ca93c828abf9 to 6784d6cd2ba194ee259f72e179e8287e0dddd5a5

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

6784d6cFull doctest coverage for Polymake pexpect interface

comment:37 Changed 4 years ago by git

  • Commit changed from 6784d6cd2ba194ee259f72e179e8287e0dddd5a5 to 8485ec834b7d3d03790c821ea06a4f4415ae56e4

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

8485ec8Be slightly clearer concerning a test being skipped

comment:38 Changed 4 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

The last commits provide full doctest coverage and passes (on my machine) both without polymake (then running sage -t) and with polymake (then running sage -t --optional=sage,polymake).

Caveats

  • I assume in the tests that the Polymake version is what is currently available by sage -i polymake: It is version 3.0. Hopefully that's fine.
  • The test for _keyboard_interact is quite flaky. Interactively, it sometimes results in a warning, sometimes in an AttributeError, sometimes it just works as stated. I have no clue concerning the reasons. Anyway, I marked it as "not tested".
  • In the test for _sage_doc_, I mention another thing I don't understand: When I do
    sage: p = polymake.rand_sphere(3,13, seed=12)
    sage: p.get_schedule?
    
    one does not get the Polymake documentation of get_schedule, even though
    sage: print p.get_schedule._sage_doc_()
    objects/Core::Object/methods/get_schedule:
    get_schedule(request;  ... ) -> Core::RuleChain
    
     Compose an optimal chain of production rules providing all requested properties.
     The returned RuleChain object can be applied to the original object as well as to any other object
     with the same initial set of properties.  If no feasible rule chain exists, `undef' is returned.
    
     To watch the rule scheduler at work, e.g. to see announcements about tried preconditions,
     you may temporarily increase the verbosity levels $Verbose::rules and $Verbose::scheduler.
    
    Arguments:
      String request : name of a property with optional alternatives or a property path in dotted notation.
        Several requests may be listed.
    
    Returns Core::RuleChain 
    
    does the right thing. In contrast,
    sage: p.minkowski_sum_fukuda?
    
    just works. Strange.

Anyway, I think it is good enough for being reviewed!


New commits:

8485ec8Be slightly clearer concerning a test being skipped

comment:39 Changed 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix

I am going through the changes right now. I will check compatibility with the last beta of polymake as well and report.

Changed 4 years ago by vdelecroix

doctest failure with polymake beta

comment:40 Changed 4 years ago by SimonKing

Thank you for testing.

The failures, as much as I see, are all harmless in the sense that simply the string representation of an object was enriched or the documentation was changed or Polymake's strategy for computing a property has changed.

What is the policy? Should the doctests for an optional package be written with respect to what you would get from the current version of the optional package, or should be "future proof" to all possible extent?

comment:41 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

first pass

  • The program is polymake and not Polymake
  • need an entry in the reference manual src/doc/en/reference/interfaces/index.rst
  • your method version is somehow broken. You are using the program polymake and not the command that was transmitted by the user. Andreas suggested
    polytope > print $Polymake::Version;
    3.0.9
    
  • you use twice os.getenv('SAGE_POLYMAKE_COMMAND') or 'polymake'. One possibility would be to set a variable named SAGE_POLYMAKE_COMMAND at the module level once for all and use it all along.
  • the install hints should give more details about polymake installation (e.g. the SPKG.txt contains some subtle details)
  • in the doctest of application I would suggest to not quit and start but rather
    EXAMPLES::
    
        sage: polymake.application('polytope')
        sage: 'tubing_of_graph' in dir(polymake)
        False
        sage: polymake.application('tropical')
        sage: 'tubing_of_graph' in dir(polymake)
        True
        sage: polymake.application('polytope')
        sage: 'tubing_of_graph' in dir(polymake)
        False
    
  • (just a remark) polymake namespaces are strange...
    sage: apps = ['polytope', 'matroid', 'graph', 'group', 'topaz', 'tropical']
    sage: for app in apps:
    ....:     polymake.application(app)
    ....:     dirs[app] = dir(polymake)
    sage: for app1 in apps:
    ....:     for app2 in apps:
    ....:         if app1 != app2: print "{:10} {:10} {}".format(app1, app2, len(set(dirs[app1]).difference(dirs[app2])))
    ....:         
    polytope   matroid    0
    polytope   graph      345
    polytope   group      338
    polytope   topaz      278
    polytope   tropical   0
    matroid    polytope   61
    matroid    graph      406
    matroid    group      399
    matroid    topaz      339
    matroid    tropical   0
    graph      polytope   0
    graph      matroid    0
    graph      group      34
    graph      topaz      0
    graph      tropical   0
    group      polytope   0
    group      matroid    0
    group      graph      41
    group      topaz      41
    group      tropical   0
    topaz      polytope   0
    topaz      matroid    0
    topaz      graph      67
    topaz      group      101
    topaz      tropical   0
    tropical   polytope   192
    tropical   matroid    131
    tropical   graph      537
    tropical   group      530
    tropical   topaz      470
    
  • in application you should check errors like
    sage: polymake.application('killerapp')
    Traceback (most recent call last):
    ...
    AssertionError: Unknown Polymake application 'killerapp'
    
    and BTW the above would better be a ValueError

comment:42 Changed 4 years ago by vdelecroix

You would better not import Polymake (upper case) in the global namespace.

comment:43 Changed 4 years ago by vdelecroix

You could test something like this with respect to the version

sage: p = Polymake(command='/ThIs/iS/NoT/PoLyMaKe')
sage: p.version()   # this should raise an error
'3.0.6'

comment:44 follow-up: Changed 4 years ago by vdelecroix

I found the method console confusing. It launches polymake but with a fresh session. I would expect to use the same console with the same variables already defined. Apparently it is a standard bad practice.

comment:45 follow-up: Changed 4 years ago by vdelecroix

Is this really what I need to use polymake.get('$myvar[0]'). Why not the cleaner polymake.get('myvar')?

comment:46 Changed 4 years ago by vdelecroix

  • There is a Math Processing Error appearing in the HTML documentation of the methods help and get_member.
  • (bike shedding) The line
    sage: p = polymake.rand_sphere(4,20, seed=5)
    
    should be
    sage: p = polymake.rand_sphere(4, 20, seed=5)
    
Last edited 4 years ago by vdelecroix (previous) (diff)

comment:47 in reply to: ↑ 44 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Apparently it is a standard bad practice.

Indeed. At least I do not follow another bad practice, which is to put all consoles into the global name space, even though there is the .console() method.

By the way, there is an .interact() method for the thing you want to do.

comment:48 in reply to: ↑ 41 Changed 4 years ago by SimonKing

Replying to vdelecroix:

first pass

  • The program is polymake and not Polymake

OK. I think one should keep the string representation of the interface instance upper case (which is consistent with everything but pari/gp).

  • your method version is somehow broken. You are using the program polymake and not the command that was transmitted by the user. Andreas suggested
    polytope > print $Polymake::Version;
    3.0.9
    

Thanks, I wasn't aware of that constant.

  • the install hints should give more details about polymake installation (e.g. the SPKG.txt contains some subtle details)

OK, I'll add: "(but read its SPKG.txt first!)"

  • in the doctest of application I would suggest to not quit and start but rather
    EXAMPLES::
    
        sage: polymake.application('polytope')
        sage: 'tubing_of_graph' in dir(polymake)
        False
        sage: polymake.application('tropical')
        sage: 'tubing_of_graph' in dir(polymake)
        True
        sage: polymake.application('polytope')
        sage: 'tubing_of_graph' in dir(polymake)
        False
    

Good idea! I asked Andreas about such examples, but he only came up with the thing that I used in the previous versions of the test.

  • in application you should check errors like
    sage: polymake.application('killerapp')
    Traceback (most recent call last):
    ...
    AssertionError: Unknown Polymake application 'killerapp'
    
    and BTW the above would better be a ValueError

OK

comment:49 in reply to: ↑ 45 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Is this really what I need to use polymake.get('$myvar[0]'). Why not the cleaner polymake.get('myvar')?

No. I tried to explain it in the NOTE on top of that test. But in the next commit that I am about to push, I'll try to be clearer in the NOTE.

comment:50 Changed 4 years ago by git

  • Commit changed from 8485ec834b7d3d03790c821ea06a4f4415ae56e4 to afdbcdb19408f1837b78a2233c0989037633cee8

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

afdbcdbTaking into account the reviewer's comments

comment:51 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

Does work with my version of polymake, and I hope it works with the next beta version as well.

Back to "needs review" :)


New commits:

afdbcdbTaking into account the reviewer's comments

Changed 4 years ago by vdelecroix

doctest at commit afdbcdb for polymake 3.0.6

Changed 4 years ago by vdelecroix

doctest at commit afdbcdb for polymake 3.0.9

comment:52 Changed 4 years ago by vdelecroix

(note: The tests on beta 3.0.6 and 3.0.9 releases do not fail in the same way)

comment:53 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

This is still wrong (see also point 4 in comment:41)

sage: from sage.interfaces.polymake import Polymake
sage: p = Polymake(command='haha')
sage: p.console()
Welcome to polymake version 3.0.6
...

comment:54 in reply to: ↑ 53 Changed 4 years ago by SimonKing

Replying to vdelecroix:

This is still wrong (see also point 4 in comment:41)

sage: from sage.interfaces.polymake import Polymake
sage: p = Polymake(command='haha')
sage: p.console()
Welcome to polymake version 3.0.6
...

I disagree.

sage: m = Magma(command="foobar")
sage: m.console()
/home/king/Sage/git/sage/local/bin/sage-native-execute: 6: /home/king/Sage/git/sage/local/bin/sage-native-execute: magma: not found

So, apparently at least one other interface is doing the same thing (it says "magma: not found" where you would expect "foobar: not found").

comment:55 Changed 4 years ago by SimonKing

Andreas created functions that help to get lists of properties, functions and member functions, and also tab completion. These functions will be in future polymake versions.

It would of course be very good if we could backport it, so that it can be used now (without the need to wait for an upgrade of the optional polymake package in Sage).

The approach would be:

  • Put Andreas' functions in a script, located in sage/ext_code
  • When starting the polymake interface, check whether Andreas' functions are already defined (which means that the user has a new polymake version)
  • If they are not defined, read the script into polymake and thus have tab completion more easily.

comment:56 Changed 4 years ago by git

  • Commit changed from afdbcdb19408f1837b78a2233c0989037633cee8 to 9a1e5f50e82a132513758f011a609803e33ae83c

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

4bb46e0Let the .console() method raise a NotImplementedError with pointers to .interact() and polymake_console()
9a1e5f5Remove test that accesses a member that soon will be removed from Polymake. Mark tests requesting documentation 'random'

comment:57 Changed 4 years ago by git

  • Commit changed from 9a1e5f50e82a132513758f011a609803e33ae83c to 7892e302c06bc518b4e858ac377dd76f1c5bdfe1

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

7892e30Replace some tests by more realistic use cases of Polymake, and mark some others as 'random'

comment:58 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

In the last few commits, I did the following:

  • When showing in the tests how to request documentation, I give the whole output as provided by polymake (which should be instructive), but mark the corresponding test as random, as the polymake documentation is of course subject to change in future versions.
  • Similar, the member function get_schedule yields output that is subject to change. Moreover, get_schedule is not something that one normally uses in polymake. So, I was marking some of the corresponding tests "random", and replaced others (in particular those that are visible in the docs) by more realistic use cases ("check whether some point is contained in a polytope").
  • Remove a test in which a member of GROUP is requested that will soon be gone.

For me (polymake-3.0) tests pass (both sage -t --optional=sage,polymake and plain sage -t). And for you, Vincent?

comment:59 Changed 4 years ago by vdelecroix

All tests pass on the two versions I have of polymake!

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:60 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Input should be INPUT. This might be the cause of the weird display of the documentation of the method Polymake.application

comment:61 Changed 4 years ago by git

  • Commit changed from 7892e302c06bc518b4e858ac377dd76f1c5bdfe1 to a18537b66b9ee27475c96268b0491fe89c8c3677

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

a18537bChange Input to INPUT

comment:62 in reply to: ↑ 60 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

Replying to vdelecroix:

Input should be INPUT. This might be the cause of the weird display of the documentation of the method Polymake.application

Done! And good that the tests even pass with the latest polymake-beta :-)

comment:63 follow-ups: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work
  • The link :meth:`~sage.interfaces.Interface.interact` in the method console is not working.
  • The display of the method application is still wrong. I think it is not safe to have a linebreak before ::. Anyway, it would be good to have EXAMPLES since TESTS do not appear in the doc. Something like
    sage: polymake.application("app1").
    sage: polymake.a_function_in_app1()
    sage: polymake.application("app2")
    sage: polymake.a_function_in_app2()
    
  • Is Evaluate of a command proper english (in the doc of _eval_line)?
  • Links in hidden methods do not appear in the doc (like _eval_line). You can either make this method appear with some sphinx directive or remove the links.
  • if not wait_for_prompt is better than if wait_for_prompt == False. elif restart_if_needed better than elif restart_if_needed==True.
  • It is hard to see that all code paths are actually tested in _eval_line. Is it reasonable to have 8 small tests that actually do that?

comment:64 in reply to: ↑ 63 ; follow-up: Changed 4 years ago by SimonKing

The patchbot complains about print statements in the doc tests. So, shall I replace all print statements by print function? Would that require from __future__ import?

Replying to vdelecroix:

  • The link :meth:`~sage.interfaces.Interface.interact` in the method console is not working.

Do you see why it doesn't?

  • The display of the method application is still wrong. I think it is not safe to have a linebreak before ::.

I thought that is what one ought to do if one doesn't like a colon to appear before a test. But I can change that.

Anyway, it would be good to have EXAMPLES since TESTS do not appear in the doc.

They don't? Ouch. I thought they used to appear in the past.

Something like

sage: polymake.application("app1").
sage: polymake.a_function_in_app1()
sage: polymake.application("app2")
sage: polymake.a_function_in_app2()

Sure. In Olot, I presented an example from the polymake tutorial that I could use here.

  • Is Evaluate of a command proper english (in the doc of _eval_line)?

Probably not...

  • Links in hidden methods do not appear in the doc (like _eval_line). You can either make this method appear with some sphinx directive or remove the links.

OK.

  • if not wait_for_prompt is better than if wait_for_prompt == False. elif restart_if_needed better than elif restart_if_needed==True.

OK. Note that I copied that from some other module (sage.interfaces.interface or sage.interfaces.expect). But I can change it anyway.

  • It is hard to see that all code paths are actually tested in _eval_line. Is it reasonable to have 8 small tests that actually do that?

I think timeout and eof isn't tested, but everything else is tested. But not all options are tested in _eval_line (e.g., polymake waiting for user input is tested in help()). Anyway I agree it makes sense to test it directly in _eval_line.

comment:65 in reply to: ↑ 63 Changed 4 years ago by SimonKing

Replying to vdelecroix:

  • Links in hidden methods do not appear in the doc (like _eval_line). You can either make this method appear with some sphinx directive or remove the links.

What would I need to do in order to make it visible in the docs? I think it SHOULD be visible.

I'll also include a test showing how to use a timeout. In fact, I needed a fix in order to make it work.

comment:66 follow-up: Changed 4 years ago by SimonKing

It seems that if the same warning is raised twice, only the first is shown. Thus, if I want to add a test in _eval_line that does the same as the test in help, then the warning is not shown.

How to prevent this?

comment:67 Changed 4 years ago by SimonKing

I found out about .. automethod::, and I think in the "warning" issue, it is fine to mark it as "random", since the output will change with future polymake versions anyway. The main point is that the interface doesn't freeze, although polymake expects user interaction.

comment:68 in reply to: ↑ 64 ; follow-up: Changed 4 years ago by vdelecroix

Replying to SimonKing:

The patchbot complains about print statements in the doc tests. So, shall I replace all print statements by print function? Would that require from __future__ import?

No, it only works at module level. We are in a crazy transition period where we want to use doctests compatible with both Python2 and Python3. So always write print(x) and avoid print(x,y).

Replying to vdelecroix:

  • The link :meth:`~sage.interfaces.Interface.interact` in the method console is not working.

Do you see why it doesn't?

There is no such object sage.interfaces.Interface.interact.

Anyway, it would be good to have EXAMPLES since TESTS do not appear in the doc.

They don't? Ouch. I thought they used to appear in the past.

In the past yes ;-)

comment:69 in reply to: ↑ 66 ; follow-up: Changed 4 years ago by vdelecroix

Replying to SimonKing:

It seems that if the same warning is raised twice, only the first is shown. Thus, if I want to add a test in _eval_line that does the same as the test in help, then the warning is not shown.

How to prevent this?

You need to set the warnings to "always" instead of "once"

https://docs.python.org/2/library/warnings.html

I am not sure it is safe to modify this configuration in doctests but it is worth giving a try.

comment:70 in reply to: ↑ 69 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Replying to SimonKing:

It seems that if the same warning is raised twice, only the first is shown. Thus, if I want to add a test in _eval_line that does the same as the test in help, then the warning is not shown.

How to prevent this?

You need to set the warnings to "always" instead of "once"

https://docs.python.org/2/library/warnings.html

I am not sure it is safe to modify this configuration in doctests but it is worth giving a try.

Since the output *after* the warning is random anyway (as it will change with future versions of polymake), I guess it is safe to mark the test as random. We only want to see that the interface doesn't hang and doesn't raise an error.

comment:71 in reply to: ↑ 68 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Replying to vdelecroix:

  • The link :meth:`~sage.interfaces.Interface.interact` in the method console is not working.

Do you see why it doesn't?

There is no such object sage.interfaces.Interface.interact.

That's a compelling reason!

Anyway, it would be good to have EXAMPLES since TESTS do not appear in the doc.

They don't? Ouch. I thought they used to appear in the past.

In the past yes ;-)

I changed some TESTS in EXAMPLES and I think in one case EXAMPLES to TEST. Commit coming soon...

comment:72 Changed 4 years ago by git

  • Commit changed from a18537b66b9ee27475c96268b0491fe89c8c3677 to f718010e5af55cfdd71810cbc92374e6b5a14ef6

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

2c88466Fix a doc link. Fix dealing with timeouts. Demonstrate how to set timeouts
f718010Add further tests, fix doc formatting

comment:73 Changed 4 years ago by git

  • Commit changed from f718010e5af55cfdd71810cbc92374e6b5a14ef6 to 0e598ac2f533471cca6fce8728de8e2ea4d6a735

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

0e598acMake it clearer that 'set()' is an internal function. Make reaction on interrupt more stable

comment:74 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

I added a test showing how to set a timeout (this required a fix).

However, I am not totally happy. While the tests related with timeout and keyboard interrupt mostly (but not always) work, they quite often fail in the doctests. I have no idea why, but I think this can wait for a subsequent ticket, as I believe the interfaces generally works.

Further ideas for the future: Implement pickling.

But for now, I think it is "needs review". The docs seem better, and also I made it clearer what methods are internal and how the interface is supposed to be used.

comment:75 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work
$ SAGE_POLYMAKE_COMMAND='NOOP' sage -t --long --optional=sage polymake.py 
Running doctests with ID 2017-03-13-20-38-15-28caf477.
Git branch: u/SimonKing/create_a_polymake_pexpect_interface
Using --optional=sage
Doctesting 1 file.
sage -t --long polymake.py
**********************************************************************
File "polymake.py", line 648, in sage.interfaces.polymake.Polymake.set
Failed example:
    c == d
Exception raised:
    Traceback (most recent call last):
    ...
    NameError: name 'c' is not defined
**********************************************************************
File "polymake.py", line 653, in sage.interfaces.polymake.Polymake.set
Failed example:
    c.VERTICES == d.VERTICES
Exception raised:
    Traceback (most recent call last):
    ...
    NameError: name 'c' is not defined
**********************************************************************
File "polymake.py", line 684, in sage.interfaces.polymake.Polymake.get
Failed example:
    polymake('cube(3)')
Exception raised:
    Traceback (most recent call last):
    ...
    TypeError: unable to start polymake
**********************************************************************
2 items had failures:
   1 of   2 in sage.interfaces.polymake.Polymake.get
   2 of   3 in sage.interfaces.polymake.Polymake.set
    [24 tests, 3 failures, 0.23 s]
----------------------------------------------------------------------
sage -t --long polymake.py  # 3 doctests failed
----------------------------------------------------------------------
Total time for all tests: 5.3 seconds
    cpu time: 0.2 seconds
    cumulative wall time: 0.2 seconds

comment:76 Changed 4 years ago by vdelecroix

However, this is fine

$ sage -t --long --optional=sage,polymake polymake.py
sage -t --long polymake.py
    [218 tests, 20.41 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

comment:77 follow-up: Changed 4 years ago by vdelecroix

There are two [Math Processing Error] in the method application.

comment:78 Changed 4 years ago by vdelecroix

There is a warning block in the documentation. Did you mean .. WARNING::? (see dev. guide)

comment:79 in reply to: ↑ 77 Changed 4 years ago by SimonKing

Replying to vdelecroix:

There are two [Math Processing Error] in the method application.

Where do you see such errors? When I tried to search for it in the logs, I couldn't find them, and the documentation looks fine.

comment:80 Changed 4 years ago by git

  • Commit changed from 0e598ac2f533471cca6fce8728de8e2ea4d6a735 to b2fd6df9e6975bb2f4b8ccbd133da97ebd4f9954

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

b2fd6dfMark three tests as optional and create a proper WARNING block.

comment:81 follow-up: Changed 4 years ago by SimonKing

The failing doctests should be marked optional. I created a .. WARNING:: block properly. And I need more information concerning the [Math Processing Error] you found.


New commits:

b2fd6dfMark three tests as optional and create a proper WARNING block.

comment:82 in reply to: ↑ 81 Changed 4 years ago by vdelecroix

Replying to SimonKing:

And I need more information concerning the [Math Processing Error] you found.

I think it is fine. My Mathjax installation was doing something bad.

comment:83 Changed 4 years ago by vdelecroix

  • Status changed from needs_work to positive_review

Good to go! Thanks for your hard work.

We shoud think about moving polymake into optional. I will open a poll on sage-devel. The main advantage is that it will be tested by default on my patchbot.

comment:84 Changed 4 years ago by vdelecroix

  • Status changed from positive_review to needs_work

Oups. Need to wait for #22501. What is the status there?

comment:85 Changed 4 years ago by jipilab

Thanks a lot Simon! Thanks a lot Vincent! I look forward to test the interface! ...once my polymake compiles...

comment:86 Changed 4 years ago by SimonKing

Trying to interactively rebase this on top of #22501, I get

king@king-ThinkPad-T430:~/Sage/git/sage$ git rebase -i t/22501/make_it_easier_to_customize_pexpect_interfaces_new 
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'
interactive rebase in progress; onto 5082c96
Last commands done (7 commands done):
   pick e801cab Change to a different approach to wrapping Perl variables
   pick 87ad586 Change syntax for catching 'ValueError as...'
Next commands to do (18 remaining commands):
   pick 69e01f3 Small change to how certain element types are printed
   pick 054123b Extending a method to use 'new' for Polymake classes
You are currently rebasing branch 't/22452/create_a_polymake_pexpect_interface' on '5082c96'.

nothing to commit, working directory clean
Could not apply 87ad58632e42a3d0c75f7871391012fd8b328f35... Change syntax for catching 'ValueError as...'

So, several commits are now missing. Since git mergetool reports that nothing needs to be merged, I don't know what to do. Please help!

comment:87 Changed 4 years ago by SimonKing

Trac sucks! I keep getting logged out after being inactive for perhaps a minute.

Anyway, I solved the problem. I just did as instructed (allowed an empty commit) and then continued rebasing. Eventually I repeated git rebase -i so that I could remove the empty commit. Pushing the result now!

comment:88 Changed 4 years ago by git

  • Commit changed from b2fd6df9e6975bb2f4b8ccbd133da97ebd4f9954 to a8157a44cc528bccd16056a0b9ec6f5401cdd1ad

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

3466f9cBe slightly clearer concerning a test being skipped
b87433fTaking into account the reviewer's comments
8f23735Let the .console() method raise a NotImplementedError with pointers to .interact() and polymake_console()
b89f2c1Remove test that accesses a member that soon will be removed from Polymake. Mark tests requesting documentation 'random'
f90647dReplace some tests by more realistic use cases of Polymake, and mark some others as 'random'
bcbabe5Change Input to INPUT
1238261Fix a doc link. Fix dealing with timeouts. Demonstrate how to set timeouts
94460aeAdd further tests, fix doc formatting
643d6b4Make it clearer that 'set()' is an internal function. Make reaction on interrupt more stable
a8157a4Mark three tests as optional and create a proper WARNING block.

comment:89 follow-up: Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

Done! So, now it can be reviewed, modulo #22501. If I recall correctly, it is possible to give a review to a ticket even though not all dependencies have received a review. Or has that changed?

comment:90 in reply to: ↑ 89 Changed 4 years ago by vdelecroix

Replying to SimonKing:

Done! So, now it can be reviewed, modulo #22501. If I recall correctly, it is possible to give a review to a ticket even though not all dependencies have received a review. Or has that changed?

At some point, Volker complained about a ticket being positively reviewed but not some of its dependencies... (do not remember the ticket number though)

comment:91 Changed 4 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to use print function in doctests

Oops, I forgot to use print function instead of print statements in the doctests.

comment:92 Changed 4 years ago by git

  • Commit changed from a8157a44cc528bccd16056a0b9ec6f5401cdd1ad to 185430f80346564983f3aeee6a7ce5643c80d4d4

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

52226e2Do not _check_valid in _repr_, since it is done in __repr__
726d3fdMerge branch 't/22501/make_it_easier_to_customize_pexpect_interfaces_new' into t/22452/create_a_polymake_pexpect_interface
185430fUse print function, not print statement, in doctests

comment:93 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues use print function in doctests deleted

The doctests are now using print function, not print statements. And for completeness, I have merged with the current version of #22501. Needs review, again.

comment:94 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:95 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long src/sage/tests/py3_syntax.py
**********************************************************************
File "src/sage/tests/py3_syntax.py", line 17, in sage.tests.py3_syntax
Failed example:
    py3_syntax.run_tests('.py')   # long time
Expected nothing
Got:
    Invalid Python 3 syntax found:
      File "src/sage/interfaces/polymake.py", line 1503
        except PolymakeError, msg:
                            ^
    SyntaxError: invalid syntax
    <BLANKLINE>
    <BLANKLINE>
**********************************************************************

comment:96 Changed 4 years ago by mkoeppe

  • Milestone changed from sage-7.6 to sage-8.0

comment:97 Changed 4 years ago by mkoeppe

  • Branch changed from u/SimonKing/create_a_polymake_pexpect_interface to u/mkoeppe/create_a_polymake_pexpect_interface

comment:98 Changed 4 years ago by mkoeppe

  • Commit changed from 185430f80346564983f3aeee6a7ce5643c80d4d4 to 52a77ee1b1fd70e0d6371f2789c4b6bfffe511ec
  • Status changed from needs_work to needs_review

New commits:

db26ccfMerge tag '7.6' into t/22452/create_a_polymake_pexpect_interface
52a77eePolymakeElement._member_list: Fix for python 3 syntax

comment:99 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:100 Changed 4 years ago by vbraun

  • Branch changed from u/mkoeppe/create_a_polymake_pexpect_interface to 52a77ee1b1fd70e0d6371f2789c4b6bfffe511ec
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:101 Changed 4 years ago by jipilab

  • Commit 52a77ee1b1fd70e0d6371f2789c4b6bfffe511ec deleted

Great!!!!! Thanks so much for all this work!!! I will let polymake people know and start showing it around!

As a follow up, I guess it would make sense to make a thematic tutorial on the interface?!?! I can volunteer to do it since I was playing a lot with the doc lately. I will continue to bug you with questions when I have them!

Thanks a lot again!

comment:102 Changed 4 years ago by vdelecroix

Indeed this is good news! I think it would be appropriate to show one example of a computation with polymake inside the thematic tutorial that you started at #22572. It can be a follow up ticket if you do not want to delay it.

Note: See TracTickets for help on using tickets.