Improve IsSubset for cyclotomic domains (such as Integers, PositiveIntegers, GaussianRationals etc.)

[limakzi]

IsSubset(Integers, [1..10^15]) previously hung because the generic fallback iterates over every element of the second argument.
Add a dedicated method for IsCyclotomicCollection and IsSemiringWithOne vs IsRange that checks containment in O(1) by inspecting the range endpoints.

Fixes #6250.

[Copilot]

Pull request overview

This PR prevents IsSubset(<cyclotomic domain>, <range>) from hanging on huge ranges (e.g. [1..10^15]) by adding a dedicated IsSubset method that decides containment in O(1) using range endpoints.

Changes:

• Install a specialized IsSubset method for IsCyclotomicCollection and IsSemiringWithOne vs IsRange in lib/cyclotom.gi.
• Add regression tests covering large ranges, descending ranges, and sign-constrained integer domains in tst/testinstall/cyclotom.tst.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
lib/cyclotom.gi	Adds an O(1) IsSubset method for cyclotomic semirings vs ranges to avoid iterating over large ranges.
tst/testinstall/cyclotom.tst	Adds regression tests ensuring the new IsSubset behavior is fast and correct across common domains and range shapes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[fingolfin]

Can you please use PR titles that are suitable for use in our release notes? In particular, no "feat:" or "fix:" etc. prefixes (we set labels to keep track of this kind of information)

[fingolfin]

(I'd also not consider this a "fix", but rather it is an enhancement: we never promised much functionality for Integers etc., so there was no bug, just missing functionality. But that's in the end of course a subjective call to make.

[limakzi]

Note argument, the description is in test file.

[fingolfin]

I don't understand what your comment is supposed to tell me.

That said: this is not correct, lists can be infinite in GAP (there is even an undeposited package InfiniteLists.

But this change ought to be safe (and capture all the cases we currently care about, so ranges and plists)

Suggested change

	[IsList,
	[IsSmallList,

[limakzi]

The comment points out that IsSubset is not symmetric with respect to its arguments here. If the first argument were IsRange, then IsSubset([-b,b], Integers) would also be slow. This is why [-b,b] is used in the test.
It might be worth adding other cases as well, such as [0,-1], [0,1], and so on, probably or create separate method for cleaness?

IsSmallList seems to be undocumented?
Could you show an example of infinite object that IsList will be true?

[fingolfin]

I would recommend keeping the test with ranges separate from that with plain lists. Both have their merit, but if they are mixed, then the this becomes obscured, and may look like an accident, not careful intent.

Indeed, IsSmallList is undocumented for "outside use" (though it has internal documentation at <lib/list.gd:116> ) but it is an integral part of the internal interface and is perfectly fine for use in library code.

Example for an infinite list:

gap> x:=Enumerator(Integers);
<enumerator of Integers>
gap> IsList(x);
true
gap> Length(x);
infinity

[fingolfin]

Actually, this might also work, I just didn't test it:

Suggested change

	[IsList,
	[IsList and IsFinite,

[limakzi]

Note test contains a test for a list as well.
The cyclotomics must not be subset of finite list.
My concern is whether we should split this test case.

[fingolfin]

I am confused as to what you are talking about here.

Ah -- is it maybe the [-b,b] at the end? That's a typo, what I meant is this:

Suggested change

	gap> ranges:=[ [-2..-1], [-1..0], [-1..1], [0..1], [1..2], [-b..-1], [-b..0], [-b..1], [-1..b], [0..b], [1..b], [-b,b]];;
	gap> ranges:=[ [-2..-1], [-1..0], [-1..1], [0..1], [1..2], [-b..-1], [-b..0], [-b..1], [-1..b], [0..b], [1..b], [-b..b]];;

[fingolfin]

I don't understand what your comment is supposed to tell me.

That said: this is not correct, lists can be infinite in GAP (there is even an undeposited package InfiniteLists.

But this change ought to be safe (and capture all the cases we currently care about, so ranges and plists)

Suggested change

	[IsList,
	[IsSmallList,

[fingolfin]

Suggested change

	InstallMethod( IsSubset, "for a list and a cyclotomic semiring",
	InstallMethod( IsSubset, "for a finite list and a cyclotomic semiring",

[fingolfin]

I am confused as to what you are talking about here.

Ah -- is it maybe the [-b,b] at the end? That's a typo, what I meant is this:

Suggested change

	gap> ranges:=[ [-2..-1], [-1..0], [-1..1], [0..1], [1..2], [-b..-1], [-b..0], [-b..1], [-1..b], [0..b], [1..b], [-b,b]];;
	gap> ranges:=[ [-2..-1], [-1..0], [-1..1], [0..1], [1..2], [-b..-1], [-b..0], [-b..1], [-1..b], [0..b], [1..b], [-b..b]];;

[fingolfin]

When I wrote that I'd suggest to keep the tests for ranges and lists apart, my idea was to insert something like this, here below the ranges tests:

Suggested change

	#
	#
	gap> lists:=[ [-2,-1], [], [-1,0,1], [0,1], [1,2], [-b,-1], [-b,0], [-b,1], [-1,b], [0,b], [1,b], [-b,b]];;
	#
	gap> SetX(ranges, lists, {r,s} -> IsSubset(s,r) = (IsEmpty(r) or (First(r) in s and Last(r) in s)));
	[ true ]
	gap> SetX(ranges, lists, {r,s} -> not IsSubset(r,s));
	[ true ]