Introduce MakeIsPlistVectorRep helper

[fingolfin]

This centralizes the creation of IsPlistVectorRep instances, reducing code duplication, and enabling future refactoring.

It also fixes bugs caused by incorrect reusing of the TypeObj of a vector to create another vector; this introduces bugs because the type can store things like the value of IsZero which must not be carelessly copied over to a new vector with different content.

Resolves #5330

TODO:

• set up benchmarks to determine the performance impact this has
• ... and then possibly work on reducing that, e.g. by caching types
• do something similar for IsPlistMatrixRep
• do something similar for IsZmodnZVectorRep
• do something similar for IsZmodnZMatrixRep

Ideally for each of the four types, only a single place calling Objectify for them would be left. This would then make further refactoring of their internal layout easy (for PR #5217)

[ThomasBreuer]

Thanks, this fixes the problem which type is taken for the results, and simplifies further improvements.
I have added a few comments.
I think this pull request should be merged soon, afterwards we can address the open questions about caching, the mutability of results, and necessary consistency checks.

[james-d-mitchell]

Wouldn't MakePlistVectorRep be a possibly better name?

[ThomasBreuer]

@james-d-mitchell concerning MakePlistVectorRep vs. MakeIsPlistVectorRep:
I am not sure what is preferable in the documentation: I feel that talking about "matrix objects" is not precise enough, "objects in the filter IsMatrixObj" is precise but long, "IsMatrixObj objects" is shorter but looks strange (like MakeIsPlistVectorRep), "MatrixObj objects" looks less strange but is not correct.

[james-d-mitchell]

Doesn't ShallowCopy return v when v is immutable, and only return a copy when v is mutable?

[ThomasBreuer]

If v is in the filter IsCopyable then ShallowCopy( v ) returns always a new object which is mutable.
If v is not in the filter IsCopyable then ShallowCopy( v ) returns v.

[james-d-mitchell]

Thanks @ThomasBreuer I didn't know this!

[ThomasBreuer]

see https://docs.gap-system.org/doc/ref/chap12_mj.html#X846BC7107C352031

(IsCopyable seems to be a not well-known concept in GAP. Perhaps the documentation should better explicitly mention IsCopyable instead of talking about "copyable objects" or "objects that support a mutable form".)

[fingolfin]

Yeah, I've been working on GAP for years but only realized in 2022 (when Thomas told me) what it really does; I somehow never had read its documentation entry. It really means IsAbleToMakeMutableInstances or CanHaveMutableInstances so (I am not proposing a renaming, just sayin')

[fingolfin]

I've removed the mutable argument for MakeIsPlistVectorRep which should take care of this and other issues.

[fingolfin]

Addressed comments, added benchmarks, added caching.

Benchmark results before the changes in this PR (beware, these timings can fluctuate quite a bit):

+---------------+
| Testing GF(2) |
+---------------+
Testing ListWithIdenticalEntries:
  8 µs per iteration; 123954 iterations per second; (200 iterations)

+-------------------+
| Testing Rationals |
+-------------------+
Testing ListWithIdenticalEntries:
  6 µs per iteration; 158724 iterations per second; (200 iterations)

+--------------------------------+
| Testing GF(2) IsPlistVectorRep |
+--------------------------------+
Testing NewZeroVector( filt, R, n ):
  24 µs per iteration; 41748 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  25 µs per iteration; 40334 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  8 µs per iteration; 117861 iterations per second; (200 iterations)

+----------------------------------+
| Testing integer IsPlistVectorRep |
+----------------------------------+
Testing NewZeroVector( filt, R, n ):
  16 µs per iteration; 62270 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  17 µs per iteration; 57292 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  8 µs per iteration; 120867 iterations per second; (200 iterations)

+-----------------------------------+
| Testing rational IsPlistVectorRep |
+-----------------------------------+
Testing NewZeroVector( filt, R, n ):
  16 µs per iteration; 64371 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  16 µs per iteration; 62485 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  8 µs per iteration; 122187 iterations per second; (200 iterations)

After the changes in this PR (so with caching):

+---------------+
| Testing GF(2) |
+---------------+
Testing ListWithIdenticalEntries:
  6 µs per iteration; 164389 iterations per second; (200 iterations)

+-------------------+
| Testing Rationals |
+-------------------+
Testing ListWithIdenticalEntries:
  6 µs per iteration; 173843 iterations per second; (200 iterations)

+--------------------------------+
| Testing GF(2) IsPlistVectorRep |
+--------------------------------+
Testing NewZeroVector( filt, R, n ):
  10 µs per iteration; 99948 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  12 µs per iteration; 85477 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  11 µs per iteration; 93609 iterations per second; (200 iterations)

+----------------------------------+
| Testing integer IsPlistVectorRep |
+----------------------------------+
Testing NewZeroVector( filt, R, n ):
  10 µs per iteration; 103500 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  11 µs per iteration; 95228 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  11 µs per iteration; 95001 iterations per second; (200 iterations)

+-----------------------------------+
| Testing rational IsPlistVectorRep |
+-----------------------------------+
Testing NewZeroVector( filt, R, n ):
  11 µs per iteration; 87841 iterations per second; (200 iterations)
Testing ZeroVector( filt, R, n ):
  12 µs per iteration; 81375 iterations per second; (200 iterations)
Testing ZeroVector( n, M ):
  12 µs per iteration; 81043 iterations per second; (200 iterations)

So the case were an "example" vector was given still is a bit slower. On the upside, the case were no example vector was given got faster.

I am going to experiment a bit with using an operation to determine where to store the cache and for initializing it, so that it becomes easier to extend this for other cases, but this might be too expensive, we'll see. But first I have to teach now :-)

[fingolfin]

BTW, I used ListWithIdenticalEntries for comparing to "old-style" vector, but perhaps more realistically would be to follow that with a call to ConvertToVectorRepNC. In that case it gets quite a bit slower:

+---------------+
| Testing GF(2) |
+---------------+
Testing ListWithIdenticalEntries:
  24 µs per iteration; 42544 iterations per second; (200 iterations)

+-------------------+
| Testing Rationals |
+-------------------+
Testing ListWithIdenticalEntries:
  12 µs per iteration; 82883 iterations per second; (200 iterations)

[fingolfin]

Re MakePlistVectorRep vs. MakeIsPlistVectorRep -- I really don't care either way, this is a purely internal helper, nobody should ever call it...

[fingolfin]

I've cleaned this up, rebased it, and marked it as "ready for review": while we still should do a similar changes for the IsPlistMatrixRep, IsZmodnZVectorRep, IsZmodnZMatrixRep, merging this ASAP would allow @ThomasBreuer to continue working on that code