turn the global variable RUN_IN_GGMBI into a global option Run_In_GGMBI

[ThomasBreuer]

resolves #6440

[ThomasBreuer]

I do not understand the time overflow for "teststandard".
Running the tests locally, I do not get big differences between the times needed by the current master branch and this pull request.
(In fact the tests ran a bit faster with the current pull request, but I do not see a reason why this should always hold.)

I am going to restart the canceled tests.

[ThomasBreuer]

In the second attempt, "teststandard" needed about 20 minutes.
Quite different from being canceled after 60 minutes.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.01%. Comparing base (83eab88) to head (7232339).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/fitfree.gi	69.23%	8 Missing ⚠️
grp/glzmodmz.gi	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6442      +/-   ##
==========================================
+ Coverage   78.89%   79.01%   +0.12%     
==========================================
  Files         685      685              
  Lines      293520   293514       -6     
  Branches     8667     8667              
==========================================
+ Hits       231558   231917     +359     
+ Misses      60162    59788     -374     
- Partials     1800     1809       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fingolfin]

Why the explicit push/pop option calls, why not simply

Suggested change

	rest:=GroupHomomorphismByImagesNC(U,Range(hom),gens,imgs);
	rest:=GroupHomomorphismByImagesNC(U,Range(hom),gens,imgs : Run_In_GGMBI);

[ThomasBreuer]

The Range call might need some computation. In the old code, this happened clearly after RUN_IN_GGMBI was set to true.
In a function call, the arguments are (currently) evaluated before the global options get set.

[fingolfin]

But Range(hom) is already being accessed in line 274?

[fingolfin]

It's not super important, I am just puzzled why in some places you use : Run_In_GGMBI := true and in others this explicit setup; OK, here it was because of the Range, and I see 1-2 more which access Source or Range of something, but otherwise the arguments all seem fine?

(I've also had a look at all our Range methods; they all should be perfectly safe; though of course in practice, for most homomorphisms the range will have been set while creating the homomorphism.)

Overall I suspect that while RUN_IN_GGMBI did indeed affect the code computing the arguments, this wasn't intentional. Perhaps @hulpke can clarify?

[ThomasBreuer]

What I tried was to replace the global variable RUN_IN_GGMBI by a global option, without side-effects.

[fingolfin]

Could also do this (but either is fine by me)

Suggested change

	Group(spec{[f..Length(spec)]}),pcgsM : Run_In_GGMBI:= true );
	Group(spec{[f..Length(spec)]}),pcgsM : Run_In_GGMBI);

[ThomasBreuer]

When trying to create test examples for the changed pieces of code, I noticed that the occurrence in ConstructFormPreservingGroup is currently not reachable.
The code would be executed if the local variable fp has a value different from fail, but this cannot happen because the groups do not store an IsomorphismFpGroup. (The code line which would set this value is commented out.)