Make holes in GAP list convert to nothing and vice versa.
#173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 71.93% 72.04% +0.11%
==========================================
Files 43 43
Lines 2302 2322 +20
==========================================
+ Hits 1656 1673 +17
- Misses 646 649 +3
|
| current_obj = ElmList(obj,i) | ||
| if haskey(recursion_dict,current_obj) | ||
| new_array[ i ] = recursion_dict[current_obj] | ||
| else |
There was a problem hiding this comment.
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)
endBut 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But btw, this is not really the scope of this PR, is it?
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
"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.
Co-Authored-By: sebasguts <[email protected]>
to return Array{Union{Any,Nothing},1}
|
I rebased the PR and applied the changes. Can we merge this? |
Co-Authored-By: sebasguts <[email protected]>
|
Thx for merging :) |
No description provided.