From 91c345c46e2da40696683e2edcf850d3de438368 Mon Sep 17 00:00:00 2001 From: ThomasBreuer Date: Thu, 12 Sep 2024 14:50:13 +0200 Subject: [PATCH] next iteration: `_needs_tracking_julia_to_gap` - add `_needs_tracking_julia_to_gap(T)`, with default value `true` - change `GAP.@install` to install a `_needs_tracking_julia_to_gap` method - change the tracking logic: - Do not *create* a dictionary for tracking identical objects only if `_needs_tracking_julia_to_gap` for the subobject type returns `false`. - Let each object decide whether it is searched in the dictionary (in the beginning of its conversion method) or whether it gets added to the dictionary (as soon as the return value is available). For the current conversion methods for integers, rationals, etc., this means that these objects are *not* added to the dictionary, that is, identical large Julia integers are converted to nonidentical large integers in GAP; if we want to change this, we have to change their `GapObj` methods. - add a method for converting `Set{T}`: This had been added to Oscar.jl but should better be in GAP.jl. (There are tests for the return value whether the result is regarded as a set by GAP; perhaps we should restrict the allowed values of `T`.) - changed `GAP.Wrappers.Add(x::GapObj, y::GapObj, z::Int)` to `GAP.Wrappers.Add(x::GapObj, y::Any, z::Int)`, in order to avoid the automatic conversion `GapObj(y)` (Once the relevant code in Oscar.jl is changed to use `GAP.@install`, the current changes in GAP.jl do not need further changes in Oscar.jl.) --- src/julia_to_gap.jl | 164 +++++++++++++++++++++++++++++++++----------- src/macros.jl | 17 +++++ src/wrappers.jl | 5 +- test/conversion.jl | 24 +++++-- test/macros.jl | 5 +- 5 files changed, 169 insertions(+), 46 deletions(-) diff --git a/src/julia_to_gap.jl b/src/julia_to_gap.jl index 4f74dc11b..8b02a9b14 100644 --- a/src/julia_to_gap.jl +++ b/src/julia_to_gap.jl @@ -62,6 +62,7 @@ The following `GapObj` conversions are supported by GAP.jl. | `Char` | `IsChar` | | `Vector{T}` | `IsList` | | `Vector{Bool}`, `BitVector` | `IsBList` | +| `Set{T}` | `IsList` | | `Tuple{T}` | `IsList` | | `Matrix{T}` | `IsList` | | `Dict{String, T}`, `Dict{Symbol, T}` | `IsRecord` | @@ -74,6 +75,23 @@ GapObj(x, cache::GapCacheDict = nothing; recursive::Bool = false) = GapObj_inter # so we must make sure it is declared before function GapObj_internal end + +# The idea behind `_needs_tracking_julia_to_gap` is to avoid the creation +# of a dictionary for tracking subobjects if their type implies that +# no such tracking is needed/wanted. +# +# We need anyhow a `_needs_tracking_julia_to_gap` method for `Any`, +# for example in order to convert a Julia object of type `Dict{Symbol, Any}` +# to a GAP record. +# Thus we have a default method returning `true`. +# +# Methods for those types that want `false` have to be installed; +# the `GapObj` methods arising from `GAP.@install` calls are unary, +# therefore the macro automatically installs such a +# `_needs_tracking_julia_to_gap` method. +_needs_tracking_julia_to_gap(::Any) = true + + GAP.@install GapObj(x::FFE) = x # Default for actual GAP objects is to do nothing GAP.@install GapObj(x::Bool) = x # Default for actual GAP objects is to do nothing @@ -145,31 +163,85 @@ function GapObj_internal( recursive::Bool, ) where {T} - if recursion_dict !== nothing && haskey(recursion_dict, obj) - return recursion_dict[obj] - end + # If I have a dictionary then the converted value may be stored. + recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj] + + # Initialize the return value. len = length(obj) ret_val = NewPlist(len) - if recursive - if recursion_dict === nothing - recursion_dict = RecDict() - end - recursion_dict[obj] = ret_val + + rec = recursive && _needs_tracking_julia_to_gap(T) + if rec && recursion_dict === nothing + # Tracking of identical subobjects is needed, + # create the dictionary. + recursion_dict = RecDict() + end + + # If we track identical subobjects then add `obj` to the dictionary. + if recursion_dict !== nothing + recursion_dict[obj] = ret_val end + + # Set the subobjects. for i = 1:len x = obj[i] if x === nothing continue end if recursive - res = get!(recursion_dict::RecDict, x) do - GapObj_internal(x, recursion_dict::RecDict, recursive) + # Convert the subobjects. + # Do not care about findíng them in the dictionary + # or adding them to the dictionary, + # since their conversion method decides about that. + if rec + res = GapObj_internal(x, recursion_dict::RecDict, recursive) + else + res = GapObj_internal(x, nothing, recursive) end else res = x end ret_val[i] = res end + + return ret_val +end + +## Sets +function GapObj_internal( + obj::Set{T}, + recursion_dict::GapCacheDict, + recursive::Bool, +) where {T} + + recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj] + + ret_val = GAP.NewPlist(length(obj)) + + rec = recursive && _needs_tracking_julia_to_gap(T) + if rec && recursion_dict === nothing + recursion_dict = RecDict() + end + + if recursion_dict !== nothing + recursion_dict[obj] = ret_val + end + + for x in obj + if recursive + if rec + res = GapObj_internal(x, recursion_dict::RecDict, recursive) + else + res = GapObj_internal(x, nothing, recursive) + end + else + res = x + end + Wrappers.Add(ret_val, res) + end + Wrappers.Sort(ret_val) + @assert Wrappers.IsSet(ret_val) + return ret_val end @@ -179,21 +251,29 @@ function GapObj_internal( recursion_dict::GapCacheDict, recursive::Bool, ) where {T} - if recursion_dict !== nothing && haskey(recursion_dict, obj) - return recursion_dict[obj] - end - (rows, cols) = size(obj) + + recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj] + + rows = size(obj, 1) ret_val = NewPlist(rows) - if recursive - if recursion_dict === nothing - recursion_dict = RecDict() - end - recursion_dict[obj] = ret_val + + rec = recursive && _needs_tracking_julia_to_gap(T) + if rec && recursion_dict === nothing + recursion_dict = RecDict() + end + + if recursion_dict !== nothing + recursion_dict[obj] = ret_val end + for i = 1:rows - # Note that we need not check whether the row is in `recursion_dict` - # because we are just now creating the object. - ret_val[i] = GapObj_internal(obj[i, :], recursion_dict, recursive) + # We need not distinguish between recursive or not + # because we are just now creating the "row objects" in Julia. + if rec + ret_val[i] = GapObj_internal(obj[i, :], recursion_dict::RecDict, recursive) + else + ret_val[i] = GapObj_internal(obj[i, :], nothing, recursive) + end end return ret_val end @@ -222,26 +302,34 @@ function GapObj_internal( recursive::Bool, ) where {S} where {T<:Union{Symbol,AbstractString}} - record = NewPrecord(0) - if recursive - if recursion_dict === nothing - recursion_dict = RecDict() - end - recursion_dict[obj] = record + recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj] + + ret_val = NewPrecord(0) + + rec = recursive && _needs_tracking_julia_to_gap(S) + if rec && recursion_dict === nothing + recursion_dict = RecDict() end + + if recursion_dict !== nothing + recursion_dict[obj] = ret_val + end + for (x, y) in obj x = Wrappers.RNamObj(MakeString(string(x))) if recursive - res = get!(recursion_dict::RecDict, y) do - GapObj_internal(y, recursion_dict::RecDict, recursive) - end + if rec + res = GapObj_internal(y, recursion_dict::RecDict, recursive) + else + res = GapObj_internal(y, nothing, recursive) + end else res = y end - Wrappers.ASS_REC(record, x, res) + Wrappers.ASS_REC(ret_val, x, res) end - return record + return ret_val end ## GAP objects: @@ -260,27 +348,25 @@ function GapObj_internal( len = length(obj) ret_val = NewPlist(len) if recursion_dict === nothing + # We have no type information that allows us to avoid the dictionary. recursion_dict = RecDict() end recursion_dict[obj] = ret_val for i = 1:len x = obj[i] - ret_val[i] = get!(recursion_dict::RecDict, x) do - GapObj_internal(x, recursion_dict::RecDict, recursive) - end + ret_val[i] = GapObj_internal(x, recursion_dict::RecDict, recursive) end elseif Wrappers.IsRecord(obj) ret_val = NewPrecord(0) if recursion_dict === nothing + # We have no type information that allows us to avoid the dictionary. recursion_dict = RecDict() end recursion_dict[obj] = ret_val for xx in Wrappers.RecNames(obj)::GapObj x = Wrappers.RNamObj(xx) y = Wrappers.ELM_REC(obj, x) - res = get!(recursion_dict::RecDict, y) do - GapObj_internal(y, recursion_dict::RecDict, recursive) - end + res = GapObj_internal(y, recursion_dict::RecDict, recursive) Wrappers.ASS_REC(ret_val, x, res) end else diff --git a/src/macros.jl b/src/macros.jl index 3f2209af7..700067b7b 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -434,6 +434,11 @@ This way, the intended `GapObj(x::T)` method becomes available, and additionally its code is applicable in recursive calls, for example when `GapObj` is called with a vector of objects of type `T`. +Since the `GapObj` method does not support a dictionary for tracking +identical subobjects, the type `T` is marked as "not needing recursion", +by automatically installing a method for `_needs_tracking_julia_to_gap` +that returns `false`. + The calls of the macro have the form `GAP.@install GapObj(x::T) = f(x)` or `GAP.@install function GapObj(x::T) ... end`. """ @@ -446,6 +451,7 @@ macro install(ex) catch error(errmsg) end + def_dict[:name] === :GapObj || def_dict[:name] == :(GAP.GapObj) || error(errmsg) length(def_dict[:args]) == 1 || error(errmsg) @@ -459,6 +465,17 @@ macro install(ex) # assemble the method definition again ex = MacroTools.combinedef(def_dict) + # install the `needs_conversion_tracking` method that returns `false` + x = def_dict[:args][1] + if x isa Symbol || !(x.head == :(::) && length(x.args) == 2) + error("argument of GapObj needs a type annotation") + else + typeannot = x.args[2] + end + Base.eval(__module__, quote + GAP._needs_tracking_julia_to_gap(::Type{$typeannot}) = false + end) + return esc(Expr( :block, :(Base.@__doc__ $ex), diff --git a/src/wrappers.jl b/src/wrappers.jl index 0852d7c13..1cdeb007b 100644 --- a/src/wrappers.jl +++ b/src/wrappers.jl @@ -3,7 +3,8 @@ module Wrappers using GAP import GAP: @wrap -@wrap Add(x::GapObj, y::GapObj, z::Int)::Nothing +@wrap Add(x::GapObj, y::Any)::Nothing +@wrap Add(x::GapObj, y::Any, z::Int)::Nothing @wrap AdditiveInverseSameMutability(x::Any)::Any @wrap Append(x::GapObj, y::GapObj)::Nothing @wrap ASS_LIST(x::Any, i::Int, v::Any)::Nothing @@ -40,6 +41,7 @@ import GAP: @wrap @wrap IsRange(x::Any)::Bool @wrap IsRangeRep(x::Any)::Bool @wrap IsRecord(x::Any)::Bool +@wrap IsSet(x::Any)::Bool @wrap IsSSortedList(x::Any)::Bool @wrap IsString(x::Any)::Bool @wrap IsStringRep(x::Any)::Bool @@ -70,6 +72,7 @@ import GAP: @wrap @wrap SetInfoLevel(x::GapObj, y::Int)::Nothing @wrap SetPackagePath(x::GapObj, y::GapObj)::Nothing @wrap ShallowCopy(x::Any)::Any +@wrap Sort(x::GapObj)::Nothing @wrap String(x::Any)::Any @wrap StringDisplayObj(x::Any)::GapObj @wrap StringViewObj(x::Any)::GapObj diff --git a/test/conversion.jl b/test/conversion.jl index 8272dad76..415a3d6d4 100644 --- a/test/conversion.jl +++ b/test/conversion.jl @@ -382,6 +382,13 @@ end @test GapObj([1 2; 3 4]) == x end + @testset "Sets" begin + x = GAP.evalstr("[1, 3, 4]") + @test GapObj(Set([4, 3, 1])) == x + x = GAP.evalstr("[\"a\", \"b\", \"c\"]") + @test GapObj(Set(["c", "b", "a"]); recursive = true) == x + end + @testset "BitVectors" begin x = GAP.evalstr("BlistList([1,2],[1])") y = GapObj([true, false]) @@ -452,23 +459,32 @@ end # a GAP list with identical Julia subobjects l = GapObj([]) x = BigInt(2)^100 + y = [1] l[1] = x l[2] = x - @test l[1] === l[2] + l[3] = y + l[4] = y res = GapObj(l) @test res[1] === res[2] + @test res[3] === res[4] res = GapObj(l, recursive=true) - @test res[1] === res[2] + @test res[1] == res[2] + @test res[1] !== res[2] # `BigInt` wants no identity tracking + @test res[3] === res[4] # `Array` wants identity tracking # a GAP record with identical Julia subobjects r = GapObj(Dict{String, String}()) setproperty!(r, :a, x) setproperty!(r, :b, x) - @test r.a === r.b + setproperty!(r, :c, y) + setproperty!(r, :d, y) res = GapObj(r) @test res.a === res.b + @test res.c === res.d res = GapObj(r, recursive=true) - @test res.a === res.b + @test res.a == res.b + @test res.a !== res.b # `BigInt` wants no identity tracking + @test res.c === res.d end @testset "converting a list with circular refs" begin diff --git a/test/macros.jl b/test/macros.jl index 71927dd4c..a230c699b 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -60,14 +60,15 @@ end end +struct TestType1 X::GapObj end +struct TestType2 X::GapObj end + @testset "@install GapObj" begin a = GAP.evalstr("(1,2)") - struct TestType1 X::GapObj end GAP.@install GapObj(x::TestType1) = x.X @test GapObj(TestType1(a)) === a - struct TestType2 X::GapObj end GAP.@install function GapObj(x::TestType2) return x.X; end @test GapObj(TestType2(a)) === a