conformance: types can't imply finiteness

[fingolfin]

Finitely presented groups can be both finite and infinite, so it is not practical to require that the type of a group must already imply whether instances of the type are finite or infinite. Yet the conformance tests were doing exactly that.

[codecov-commenter]

Codecov Report

Merging #41 (668134f) into main (d52268f) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   96.39%   96.36%   -0.04%     
==========================================
  Files           4        3       -1     
  Lines         111      110       -1     
==========================================
- Hits          107      106       -1     
  Misses          4        4              

Flag	Coverage Δ
unittests	96.36% <ø> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d52268f...668134f. Read the comment docs.

[kalmarek]

I think this testset hasn't been finished:

elseif IS isa Base.IsInfinite
     ...
else
    @test IS isa Base.SizeUnknown
    @test_throws ArgumentError isfinite(G)
end

[fingolfin]

That also makes no sense. IS is derived only from the type of G. But there are types which have both finite and infinite instances. So isfinite will not throw an exception in general.

[kalmarek]

then surround @test_throws by try statement and test first whether isfinite returns a Bool

[fingolfin]

I've split this PR into three; this original PR retains the iterator test changes. I've now adjusted them along the lines of what you wrote, but in a way that ensures the tests for finite groups are performed solely based on whether isfinite(G) == true, regardless of what Base.IteratorSize(typeof(G)) gives.

[kalmarek]

Just because elements are deepcopiable doesn't mean that asking for the
identiy object multiple times will give non-identical results. Not even a
deepcopy need be non-identical, if the group element type is a struct

I can follow the first part, but then the fix is not to delete the test, but replace it with

@test deepcopy(one(G)) !== one(G)

The second part I can't really follow though. Objects that can be deepcopied are precisely those which can be made distinct by... deepcopying them. And the definition _is_deepcopiable(g) = !isbits(g) says precisely that these tests won't be run on stack-allocated (is this what you mean by struct?) hence not deepcopiable objects. Btw. I vaguely recall that this function/test guards were added precisely because of problems with GAPGroupElements, though I can't find the discussion now. Do tests pass if you GroupsCore._is_deepcopiable(::GAPGroupElemen) = false?

Giving it a second look, probably the the guard function should be defined in a separate file in tests, there is no need for it in the package itself.

[fingolfin]

The relation you claim between isbits/isbitstype on the one hand and deepcopying on the other simply does not hold in reality: there are struct types which are not bits types but for which deepcopy nevertheless returns identical objects. Eg in Oscar, groups elements are structs which are not bits types because they contain a parent pointer. But deepcopy preserves the parent, hence === does not distinguish the original object from its deepcopy

[fingolfin]

To give a concrete example:

julia> struct B
         x::Int
         v::Vector{Int}
         B(x::Int=0) = new(x)
       end

julia> b = B()
B(0, #undef)

julia> isbits(b)
false

julia> isbitstype(B)
false

julia> deepcopy(b) === b
true

All in all, it is also is not clear to me what this check on deepcopy is supposed to achieve? Hence I just removed it in this PR.

[kalmarek]

groups elements are structs which are not bits types because they contain a parent pointer. But deepcopy preserves the parent, hence === does not distinguish the original object from its deepcopy

Yes I know about GAPGroupElements and hence _is_deepcopiable was introduced for you to redefine it ;). What happens if you define

GroupsCore._is_deepcopiable(::GAPGroupElement) = false

?

All in all, it is also is not clear to me what this check on deepcopy is supposed to achieve?

The purpose is to make sure that if a group element is deepcopiable then deepcopy_internal (which the user must implement) works as intended = a deepcopy of a deepcopiable element is truly distinct.

I like the example ;) though it seems to be a corner case, since deepcopy_internal only considers the fields which are assigned (there can be only so many nulls :P). We can fix the definition of _is_deepcopiable to

_is_deepcopiable(g::GroupElement) = !isbits(g) && all(nf->isdefined(g, nf), fieldnames(typeof(g)))

[fingolfin]

I've split this PR into three parts, so that they can be discussed and possibly merged independently. Let's move the discussion on deepcopy to PR #43. Should have done that from the start :-)

[fingolfin]

ah this should also verify that isfiniteG is a Bool. Will add.

[lgoettgens]

@kalmarek Can we get a release containing this PR? We would like to use the conformance tests for generalized Weyl groups in Oscar, but are currently unable to do so as our groups can both be finite and infinite. This PR should fix this.

cc @felix-roehrich

[kalmarek]

@lgoettgens sure; Since you've implemented a Group interface what is your opinion on #44 ? Can you show a link to your implementation?