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.

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

WIP: rewrite gap_to_julia #777

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jan 24, 2022

TODO:

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #777 (7b68aa5) into master (6bafdba) will increase coverage by 2.57%.
The diff coverage is 96.74%.

❗ Current head 7b68aa5 differs from pull request most recent head ea7e465. Consider uploading reports for the commit ea7e465 to get more accurate results

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
+ Coverage   75.73%   78.31%   +2.57%     
==========================================
  Files          51       46       -5     
  Lines        4159     3652     -507     
==========================================
- Hits         3150     2860     -290     
+ Misses       1009      792     -217     
Files Changed Coverage
src/gap_to_julia.jl 96.66%
src/constructors.jl 100.00%

TODO: document that `type` must be a `Any` or a concrete type, and likewise
for its type parameters. This means that `Tuple{Any}` is not allowed, nor
are unions. Indeed, for a union, how should we determine which of the various
union types should be the end result of the conversion?
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand this statement.
Currently we have

julia> GAP.gap_to_julia(GAP.evalstr("[1,Z(2),true]"))
3-element Vector{Any}:
    1
     GAP: Z(2)^0
 true

and

julia> Vector{GAP.Obj}(GAP.evalstr("[1,Z(2),true]"))
3-element Vector{Union{Bool, GAP.FFE, GAP.GapInt}}:
    1
     GAP: Z(2)^0
 true

Shall the latter be forbidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

So we already discussed this on gather, but I want to record what I said here for others / myself in the future:

Indeed the text is not phrased correctly. What I wanted to express is that I don't see how to handle a request like this properly:
Vector{Union{String,Int,Symbol}}(GAP.evalstr("[1, \"x\"]"))

In principle this could be dealt with, in that there are sensible conversion results of the requested type for this, namely [1, "x"] and [1, :x]. But then we need to decide which of them to return? Perhaps do it arbitrarily? Or introduce some kind of priority between certain types (e.g "if both Symbol and String are requested and possible, prefer Symbol"), but then how to extend that to all cases (We could be asked to produce a Vector{Union{Vector{Vector{Int}}, Matrix{Int}, fmpz_mat, fmpq_mat} ...).

In contrast, conversion to Vector{GAP.Obj} above works, despite GAP.Obj being a union type, because all the elements of the GAP list already had a type in that Union. So in that case the "trivial conversions" rule applies, i.e., asking to convert an object x::T to type S always succeeds and returns x whenever S is either T or a supertype of T.


# Union types need tracking if any of the types the union was formed over need it
#_needs_conversion_tracking(T::Union) = any(_needs_conversion_tracking, Base.uniontypes(T))
# TODO: alternatively just handle GAP.GapInt, GAP.Obj
Copy link
Member

@ThomasBreuer ThomasBreuer Jan 25, 2022

Choose a reason for hiding this comment

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

As far as I understand, in line 46 above, it is stated that union types are not allowed.

TODO:
- add more test cases esp. for the various fixes
  e.g. converting the same (identical) objects to multiple
  output types should be working correctly now
- identify issues this patch resolves
- update docs
- ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaw in recursive conversion in gap_to_julia
2 participants