Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make holes in GAP list convert to nothing and vice versa. #173

Merged
merged 6 commits into from
Nov 29, 2018
Merged

Make holes in GAP list convert to nothing and vice versa. #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
efddb9e
Conversion of lists with holes
sebasguts Nov 23, 2018
4625fb7
Handle nothing specially in julia array conversion
sebasguts Nov 23, 2018
fa8a36b
Spacing in comments
fingolfin Nov 24, 2018
9d1e732
Changed default IsList conversion
sebasguts Nov 27, 2018
719ad75
Update comment in LibGAP.jl/src/gap_to_julia.jl
fingolfin Nov 28, 2018
dc2fe11
Fixed nothing return in gap_to_julia
sebasguts Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions LibGAP.jl/src/ccalls.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ function CharWithValue(x::Cuchar)
return o
end

function ElmList(x::MPtr,position)
o = ccall( :GAP_ElmList,
Ptr{Cvoid},
(MPtr,Culong),
x,Culong(position))
return GET_FROM_GAP(o)
end

"""
(func::MPtr)(args...)

Expand Down
31 changes: 28 additions & 3 deletions LibGAP.jl/src/gap_to_julia.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ of converted objects and should never be given by the user.
gap_to_julia(::Type{GAPInputType},x::GAPInputType) = x
gap_to_julia(::Type{Any}, x::GAPInputType) = gap_to_julia(x)
gap_to_julia(::Type{Any}, x::Any ) = x
gap_to_julia(::Any, x::Nothing ) = nothing
fingolfin marked this conversation as resolved.
Show resolved Hide resolved
gap_to_julia(::Type{Any}, x::Nothing ) = nothing

## Integers
gap_to_julia(::Type{Int128} ,x::Int64) = trunc(Int128 ,x)
Expand Down Expand Up @@ -125,7 +127,7 @@ function gap_to_julia( ::Type{Array{Obj,1}}, obj :: MPtr , recursion_dict = IdDi
new_array = Array{Any,1}( undef, len_list)
recursion_dict[obj] = new_array
for i in 1:len_list
current_obj = obj[i]
current_obj = ElmList(obj,i) # returns 'nothing' for holes in the list
if haskey(recursion_dict,current_obj)
new_array[ i ] = recursion_dict[current_obj]
else
Expand All @@ -147,7 +149,30 @@ function gap_to_julia( ::Type{Array{T,1}}, obj :: MPtr, recursion_dict = IdDict(
new_array = Array{T,1}( undef, len_list)
recursion_dict[obj] = new_array
for i in 1:len_list
current_obj = obj[ i ]
current_obj = ElmList(obj,i)
if haskey(recursion_dict,current_obj)
new_array[ i ] = recursion_dict[current_obj]
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this is what we want to do or not... see also PR #178... We need to write down the rules for who is responsible for setting recursion_dict entries, and who for checking recursion_dict entries: the caller or the callee. Right now, it seems to be a mix: Some methods check at the start whether the given object is in recursion_dict, and if so, return the value it maps to. OTOH, some functions check before performing recursive conversion.

The easiest to write is to add if haskey(recursion_dict,obj) then return obj end at the start of each conversion function. But then we need to do that even if we don't have a recursive conversion, and we need it even if we just convert some ints.

So the next idea was to instead perform this check inside any conversion function for container types, like arrays/dicts resp. lists/records. But we should not do both.

If we do that, we want to be careful, so that e.g. two identical GAP large ints still get converted into two identical Julia bigints (although arguably, this is less important than it is for container types).

I guess we could recover the first approach w/o loosing much or anything, if we just modify the fallback method

julia_to_gap(obj::Any, recursive, recursion_dict ) = julia_to_gap(obj)

to instead look like this:

function julia_to_gap(obj::Any, recursive, recursion_dict )
    if haskey(recursion_dict,obj)
        return recursion_dict[obj]
    end
    return julia_to_gap(obj)
end

But then of course any proper 3-arg method for julia_to_gap still needs to contain that check...

So I am still not completely sure where placing that check makes most sense, but I do know that we should do it either-or, not both.

Anyway, there is another related question: If we have a list that contains itself, like the GAP list l:=[~]. Then it is clear how we want to handle it for recursive conversions. But what about non-recursive? In the current approach, what happens is that as we iterate over the content of l, we discover that its first entry was already converted, and use that result; so we end up creating a Julia list containing itself, even if recursion is off. I'd argue this is wrong, because it is inconsistent with the result for l := [ [ ~ ] ]: This list also contains itself, but one level deeper down. Since we don't recurse, we never get there, and thus the reference to the original GAP list is retained. It seems odd that this would work differently here.

Assuming that we do not want the if haskey(recursion_dict,obj) at the start of every conversion function, this suggests the following kind of logic for recursive conversions:

  ...
  current_obj = ...
  if Recursive
    if haskey(recursion_dict,current_obj)
      current_obj = recursion_dict[current_obj]
    else
      current_obj = recursion_dict[current_obj] = gap_to_julia(T, current_obj, recursion_dict)
    end
  end
 # ... now put current_obj into the container we are creating...

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of the variant where the caller handles the recursion. While I think both is feasible, it seems more natural to me that the caller checks the recursion.

I would guess for the non-recursive conversion, we can skip the recursion alltogether.

Copy link
Contributor Author

@sebasguts sebasguts Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But btw, this is not really the scope of this PR, is it?

Copy link
Member

@fingolfin fingolfin Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the callers handles the conversion" is what I did for julia_to_gap in PR #178, so it seems we agree.

And I do think that my comments here are at least somewhat relevant to this PR, because this PR changes the recursion code for gap_to_julia, and I think that code is incorrect per our common understanding how recursive conversion should work. Of course it may have been incorrect before; but in any case, when reviewing this PR, I noticed the problem.

Specifically, I'd argue that if one wants to non-recursively convert the GAP self-containing list [~], the result should be a Julia list containing that GAP list. But that's not the case right now...

So, while this PR does not introduce a new issue, it modifies code with an existing issue; and a fix for that will necessarily conflict with this PR. So, let's merge this PR first, then work on a fix.

Copy link
Contributor Author

@sebasguts sebasguts Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the callers handles the conversion" is what I did for julia_to_gap in PR #178, so it seems we agree.

Yep.

And I do think that my comments here are at least somewhat relevant to this PR, because this PR changes the recursion code for gap_to_julia,

I do not see where I actually changed the way recursion is handled in this PR.

This is not against your comments whatsoever, they make perfect sense and are important. But I would still rather discuss it in a new PR.

new_array[ i ] = gap_to_julia(T,current_obj,recursion_dict)
recursion_dict[ current_obj ] = new_array[ i ]
end
end
return new_array
end

## Special case for conversion of lists with holes; these are converted into 'nothing'
function gap_to_julia( ::Type{Array{Union{Nothing,T},1}}, obj :: MPtr, recursion_dict = IdDict() ) where T
if ! Globals.IsList( obj )
throw(ArgumentError("<obj> is not a list"))
end
if haskey(recursion_dict,obj)
return recursion_dict[obj]
end
len_list = length(obj)
new_array = Array{Union{Nothing,T},1}( undef, len_list)
sebasguts marked this conversation as resolved.
Show resolved Hide resolved
recursion_dict[obj] = new_array
for i in 1:len_list
current_obj = ElmList(obj,i)
if haskey(recursion_dict,current_obj)
new_array[ i ] = recursion_dict[current_obj]
else
Expand Down Expand Up @@ -215,7 +240,7 @@ function gap_to_julia(x::MPtr)
elseif Globals.IsString(x)
return gap_to_julia(AbstractString,x)
elseif Globals.IsList(x)
return gap_to_julia(Array{Any,1},x)
return gap_to_julia(Array{Union{Any,Nothing},1},x)
elseif Globals.IsRecord(x)
return gap_to_julia(Dict{Symbol,Any},x)
end
Expand Down
3 changes: 3 additions & 0 deletions LibGAP.jl/src/julia_to_gap.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ function julia_to_gap(obj::Array{T,1}, recursive::Val{Recursive}=Val(false), rec
end
for i in 1:len
x = obj[i]
if x == nothing
continue
end
if Recursive
#=
# It would be much nicer to write this, but we cannot in Julia 1.x, see
Expand Down
13 changes: 13 additions & 0 deletions LibGAP.jl/test/conversion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,15 @@
xx = GAP.julia_to_gap( "a" )
@test_throws MethodError GAP.gap_to_julia( Dict{Int64,Int64}, xx )

## Test converting GAP lists with holes in them
xx = GAP.EvalString( "[1,,1]" )
@test GAP.gap_to_julia(xx) == Any[1,nothing,1]
@test GAP.gap_to_julia(Array{Any,1},xx) == Any[1,nothing,1]
@test_throws MethodError GAP.gap_to_julia(Array{Int64,1},xx)
@test GAP.gap_to_julia(Array{Union{Nothing,Int64},1},xx) == Union{Nothing,Int64}[1,nothing,1]
@test GAP.gap_to_julia(Array{Union{Int64,Nothing},1},xx) == Union{Nothing,Int64}[1,nothing,1]


end

@testset "conversion to GAP" begin
Expand Down Expand Up @@ -213,4 +222,8 @@ end
conv = GAP.julia_to_gap(d, Val(true));
@test conv === conv.b

## Test converting lists with 'nothing' in them -> should be converted to a hole in the list
xx = GAP.EvalString( "[1,,1]" )
@test GAP.julia_to_gap([1,nothing,1]) == xx

end