Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

JConvex: systematically deal with rational and big coefficients #75

Closed
fingolfin opened this issue Oct 8, 2021 · 0 comments · Fixed by #76
Closed

JConvex: systematically deal with rational and big coefficients #75

fingolfin opened this issue Oct 8, 2021 · 0 comments · Fixed by #76

Comments

@fingolfin
Copy link
Member

When I refactored JConvex, I replaced code like this:

        s := JuliaToGAP( IsString, Julia.string( cone!.pmobj.RAYS ) );
        res_string := SplitString( s, '\n' );
        res_string := List( [ 2 .. Length( res_string ) ], i -> Concatenation( "[", ReplacedString( res_string[ i ], " ", "," ), "]" ) );
        rays := EvalString( Concatenation( "[", JoinStringsWithSeparator( res_string, "," ), "]" ) );

by stuff like this:

        rays := JuliaToGAP( IsList, JuliaMatrixInt( cone!.pmobj.RAYS ) );

where JuliaMatrixInt really is the Julia type Matrix{Int}.

I think that was a mistake. We probably should be using Matrix{Rational{BigInt}}. Because JConvex then often proceeds with code like this:

        # sometimes, Polymake returns rational rays - we turn them into integral vectors
        scaled_rays := [];
        for i in [ 1 .. Length( rays ) ] do
            scale := Lcm( List( rays[ i ], r -> DenominatorRat( r ) ) );
            Append( scaled_rays, [ scale * rays[ i ] ] );
        od;

From @lkastner I learned that one can let Polymake do this scaling, by calling Polymake.common.primitive. So perhaps even better would be to replace all that scaling code by something like that (note the use of a to-be-defined JuliaMatrixBigInt):

        rays := _Polymake_jl.common.primitive( cone!.pmobj.RAYS ); # ensure integer coeffs
        rays := JuliaToGAP( IsList, JuliaMatrixBigInt( rays ) );

and then `scaled_rays is superfluous.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant