Work on conversion

[sebasguts]

For a detailed description on this work, see https://hackmd.io/-xkD8BtORfCcgi-TQsb8Eg?both

Resolves #162
Resolves #146
Resolves #116

[codecov]

Codecov Report

Merging #160 into master will decrease coverage by 11.19%.
The diff coverage is 84.73%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   65.59%   54.39%   -11.2%     
==========================================
  Files          39       37       -2     
  Lines        2334     1682     -652     
==========================================
- Hits         1531      915     -616     
+ Misses        803      767      -36

Impacted Files	Coverage Δ
JuliaInterface/gap/JuliaInterface.gd	100% <ø> (ø)	⬆️
JuliaInterface/src/calls.h	100% <ø> (ø)	⬆️
LibGAP.jl/src/initialization.jl	92.3% <ø> (-0.29%)	⬇️
JuliaInterface/gap/calls.gi	100% <100%> (ø)
JuliaInterface/src/JuliaInterface.c	95.85% <100%> (+0.2%)	⬆️
JuliaInterface/src/calls.c	99.08% <100%> (-0.19%)	⬇️
JuliaInterface/gap/convert.gd	100% <100%> (ø)
LibGAP.jl/src/gap.jl	100% <100%> (ø)	⬆️
JuliaInterface/src/convert.c	100% <100%> (ø)	⬆️
JuliaInterface/read.g	100% <100%> (ø)	⬆️
... and 37 more

[sebasguts]

@fingolfin @ThomasBreuer @frankluebeck I think this is finished for now. Sorry for the mess. Please have a look and review it.

@ThomasBreuer JuliaExperimental is not completely adjusted yet.

[fingolfin]

Please add a longer comment to this file which explains what is going on. Like: What is this Val{Recursive} bit doing; but also the general idea of what kind of methods, with what signature, one has to achieve what.

I actually need that explanation, as I don't understand right now why this complicated setup is better than this simpler one: For all "basic" types (= not containing subobjects that need to be converted), have methods like this:

julia_to_gap(x::Int32, recursion_dict) = Int64(x)

Then, have one generic redispatch function which handles all these primitive ones at once; and for all data types with subobjects, have both 1 and 2 arg methods:

julia_to_gap(x::Any) = julia_to_gap(x, nothing)

function julia_to_gap(obj::Array{T,1}, recursion_dict = IdDict()) where T
...
end

Perhaps the code the above produces is suboptimal? Well, I wouldn't ask this question if there was a comment explaining the design of the current approach :-)

[sebasguts]

I am not sure about the code being less optimal that way. I will try to implement the changes you propose.

[fingolfin]

According to Codecov, there are no tests for this function at all, it is never called :-( Same for the other rationals conversion above, and the float conversion below, and more -- see also https://codecov.io/gh/oscar-system/GAPJulia/pull/160/diff

[frankluebeck]

(side remark: is recusive_dict a typo?)

[fingolfin]

Should there perhaps also be a reverse conversion for this? But of course that's difficult, as on the GAP side, infinity and -infinity are objects, and "only" in the filter IsCyc, not in IsRat. Huh :/

[ThomasBreuer]

@fingolfin As far as I remember,
the object infinity had originally been designed just for having a natural object
that is larger than any integer, and that can be compared with integers via = and <.
In particular, there were no arithmetic operations for infinity,
and also -infinity did not yet exist.
Because of the wish to compare the new object infinity with integers,
the family CyclotomicsFamily was a natural choice for it,
and thus infinity is in the filter IsCyclotomic --but not in IsCyc, IsRat, or IsInt.

Before the introduction of infinity, other GAP objects of different types had been used
for this kind of comparison, for example E(3),
assuming that GAP regards them as larger than integers.
Using infinity in such situations is nicer because it is self-explanatory;
besides that, I do not think that infinity is useful.

[fingolfin]

I am not sure how this is relevant for the discussion at hand, though?

BTW, I added -infinity, and I find it and infinity quite useful for all kinds of things, e.g. for polynomial degree functions, or arbitrary valuations; as default values for minimum / maximum operations; etc.. But whether one finds them useful or not is kind of irrelevant for this discussion. What we have to deal with is issue #162. We can either throw an error if we encounter the Julia rationals 1//0 and -1//0; or convert them to infinity and -infinity (as this PR does). In the latter case, the question arises whether we want to make the conversion roundtrip or not. While in general, I am in favor for clean roundtrip conversions, but is of course not always possible; but then perhaps there should be a comment here pointing that out.

[sebasguts]

I think it is sensible to convert to infinity here.

Remember, we are talking about a very specific corner-case here, Julia rationals with 0 as denominator. I do not think it is that important anyway.

[frankluebeck]

In the moment I cannot try the code from this pull request (I get an error when trying to load the package). So, here are some general comments based on this description (which I have edited slightly).

• Mappings between GAP's IsString and Julia's AbstractString look wrong to me. As far as I understand the documentation of the latter these are Unicode strings in some encoding. But in GAP strings can contain arbitrary binary data, and even many strings which are related to text, e.g., those used by the help system, are not Unicode or UTF-8 encoded strings.
• For more basic types like multiprecision integers: There are several implementations of these available on the Julia side. Is there a rationale to choose Julia's BigInt in the automatic conversions and not, e.g., FLINTs fmpz?
• Lists: how are GAP's lists with holes handled in the automatic conversions? Is there an equivalent on the Julia side? This must be addressed in the description.
• Mutable/Immutable lists and records in GAP: how is this reflected in the automatic conversions to Julia objects?

[fingolfin]

@frankluebeck "I get an error when trying to load the package" -> could you perhaps tell us what that error is?

[frankluebeck]

I was loading "JuliaExperimental" which is maybe not expected to work with this pull request. Loading "JuliaInterface" only does work.

So, now I get:

gap> s:="";;
gap> for i in [0..255] do Add(s,CharInt(i));od;
gap> js:=GAPToJulia(s);
<Julia: "">
gap> sjs:=JuliaToGAP(IsString,js);
""
gap> Length(sjs);
0
gap> GAPToJulia([1,,2]);
Error, List Element: <list>[2] must have an assigned value
not in any function at *stdin*:19
you can 'return;' after assigning a value
brk> 

[fingolfin]

1. While String is indeed restricted to UTF-8, AbstractString explicitly is meant to support arbitrary encodings. But of course we actually create String instances, so I guess we might need to use or introduce a custom RawString or `GAPString class instead. Or perhaps somebody has a better idea?

2. Automatic conversions do not convert bigints. For the explicit manual conversions, we do indeed provide a generic conversion between GAP large ints and Julia big ints. For non-recursive conversions, I think that's fine, as you can simply invoke another conversion function if you want, say, FLINT bigints instead. But I guess if you have a list of GAP large ints, it'd be nice if one could quickly convert that to a list of FLINT bigints, too... And in a sense, you can, using functions like map. But I agree this needs some more thought.

3. Right now, lists with holes are not supported, good point. We could of course reject them; but we could also create lists containing nothing in place of the hole, which would be nicely consistent with julia_gap mapping NULL pointers to nothing.

4. Mutability is not yet addressed at all, neither in automatic nor in manual/explicit conversions. There is likely no perfect solution for this either; e.g. Julia's AbstractString is always immutable, just like Java strings.

[fingolfin]

Yes, JuliaExperimental is currently broken

[sebasguts]

1. There is a Cstring class in Julia, which is composed of Cchars. It is basically meant to be compatible with char*. The other way around is a problem, though. Julia strings are by default Strings, which are UTF-8 encoded. If we want to not have any pitfalls here, we would probably need a custom GAP object. If we want to convert easily, I think AbstractString is the right thing to do.

2. This is merely a problem about how we convert lists: We use GAP's ELM_LIST function, which errors if the element is not set. We could use a function that may return nothing instead. But note that this would make the conversion different. If you want to convert to an Array{Int64}, we would need to set the returning type to Array{Union{Int64,Nothing}} - So either one must know before that the list might have holes, or we have to return a different type than what we specified.

3. Indeed, Julia does not have mutable/immutable variants of all objects. Do we really want to preserve mutability all the time?

[fingolfin]

From my POV, these are great changes into the right direction. Of course there are still things we can improve, but let's merge this first, then iterate -- e.g., fix JuliaExperimental, merging PR #158 (after it gets rebased and improved bit), add some more tests, start writing some docs that explain how JuliaInterface and LibGAP.jl fit together, etc. But best to do that in many short lived feature branches, instead of growing this big bad branch even more.