fix in ShortestVectors for issue #838

[dimpase]

See #838 for details and an example to check.

This is needed if "positive" is set in a call to OrthogonalEmbeddings and the last vector c.vectors[anz] produced by the function ShortestVectors happens to be not non-negative. Then the lines rejecting it:

if checkpositiv and neg then
       c.vectors[anz] := [];
       anz := anz - 1;

leave an uncleared []last entry in c.vectors, resulting in the error mentioned in the issue at hand.

[olexandr-konovalov]

Note that Thomas suggested in the Support list that "For correcting the bug, it would be sufficient to call 'Unbind' instead of assigning an empty list [in the local function 'vschr' inside the function 'ShortestVectors']; however, it would be better to rewrite the function completely."

[dimpase]

@alex-konovalov : calling Unbind in a loop creates a lot of work for GASMAN, and it's not necessary. My patch produces an equivalent result and does not do this.

[frankluebeck]

@dimpase: How can the creation of unnecessary objects and their assignment to list entries be more efficient for the garbage collection than just unbinding that list entry?

Also, your proposed change may cause the creation of an unnecessary shallow copy of the list in .vectors, e.g. in the usual case where this function is not called with the "positive" argument.

[dimpase]

@frankluebeck : there is at most one unnecessary object (list entry in .vectors) created in the current loop implementation, and it should stay this way.

Although I agree that my one-liner might be more efficiently implemented as conditional unbinding of the last entry of the list in .vectors. If this would be acceptable as the fix I can do this.

[markuspf]

Any (supposed) performance implication can surely easily be shown to (not) exist by an appropriate test and benchmark.

I wonder though what kind of "complete rewrite" is meant.

[olexandr-konovalov]

Just to remind everyone - this is still open, and we need to review this and either approve or submit an alternative solution.

[dimpase]

How much effort does it take to review a 1-line patch clearing out an empty last entry of a list, produced under certain conditions I explained in some detail above? I'm turning 53 today, give me a birthday present by reviewing it :-)

[ChrisJefferson]

I don't understand why you aren't using Unbind, if you just want to get rid of a value. I can assure you, Unbind has no cost, beyond setting a pointer to 0.

[markuspf]

That example in #838 takes ages to verify. Do you have a shorter running one? This needs test-cases that don't run for minutes (or hours).

In particular if anyone is going to try to "rewrite the function completely" at any point.

[dimpase]

There is an example suggested there that takes about 5 seconds on a modern laptop:

g:=
[ [ 12, 4, 4, 4, 4, 4, 0, 0, 0, 0 ], [ 4, 8, 0, 0, 0, 0, 0, 0, 0, 0 ],
  [ 4, 0, 8, 0, 0, 0, 4, 0, 0, 0 ], [ 4, 0, 0, 8, 0, 0, 0, 4, 0, 0 ],
  [ 4, 0, 0, 0, 8, 0, 0, 0, 4, 0 ], [ 4, 0, 0, 0, 0, 8, 0, 0, 0, 4 ],
  [ 0, 0, 4, 0, 0, 0, 12, 0, 0, 0 ], [ 0, 0, 0, 4, 0, 0, 0, 12, 0, 0 ],
  [ 0, 0, 0, 0, 4, 0, 0, 0, 12, 0 ], [ 0, 0, 0, 0, 0, 4, 0, 0, 0, 12 ] ];;
m:=10;; r := OrthogonalEmbeddings((1/2)*g{[1..m]}{[1..m]}, "positive");

(and on unpatched code it finishes in no time with the error)

Please tell me where is should go in the testsuite, I'll add it there.

[dimpase]

@ChrisJefferson : I prefer functional style code, that's why.