Revised list of packages loaded by default

[olexandr-konovalov]

This PR comes from issue #906 and is submitted to check how continuous integration tests respond if some packages are excluded from the list of packages loaded by default when GAP starts. To start with, I've removed LAGUNA, Sophus and ResClasses.

[codecov]

Codecov Report

Merging #1135 into master will decrease coverage by -0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
- Coverage   57.11%   57.08%   -0.03%     
==========================================
  Files         434      433       -1     
  Lines      224798   224574     -224     
==========================================
- Hits       128391   128202     -189     
+ Misses      96407    96372      -35

Impacted Files	Coverage Δ
lib/package.gi	34.82% <ø> (-1.44%)	❌
src/weakptr.c	82.81% <ø> (-7.82%)	❌
src/tietze.c	82.37% <ø> (-6.88%)	❌
lib/overload.g	25% <ø> (-6.82%)	❌
src/vars.c	62.02% <ø> (-4.21%)	❌
lib/field.gi	36.49% <ø> (-4.03%)	❌
lib/addcoset.gi	27.77% <ø> (-2.78%)	❌
src/vecgf2.c	60.83% <ø> (-1.25%)	❌
lib/info.gi	45.08% <ø> (-0.82%)	❌
... and 14 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 c5272cb...1110531. Read the comment docs.

[olexandr-konovalov]

Travis tests pass without LAGUNA, Sophus and ResClasses. Next commit also removes Crisp, Polenta and Polycyclic.

[olexandr-konovalov]

Because Alnuth needs polycyclic and Irredsol suggests Crisp, the visible effect of c0e8316 only includes Polenta package:

Packages:   Alnuth 3.0.0, AtlasRep 1.5.1, AutPGrp 1.6, CRISP 1.4.4, 
            CTblLib 1.2.2, FactInt 1.5.3, FGA 1.3.1, GAPDoc 1.5.1, IO 4.4.6, 
            IRREDSOL 1.3.1, Polycyclic 2.11, SpinSym 1.5, TomLib 1.2.6 

[olexandr-konovalov]

So, now we know that Travis tests pass with

default:= [ "autpgrp", "alnuth", "ctbllib", "factint", "fga", "irredsol", "tomlib" ],

with the actual list of packages loaded due to dependencies is

Packages:   Alnuth 3.0.0, AtlasRep 1.5.1, AutPGrp 1.6, CRISP 1.4.4, 
            CTblLib 1.2.2, FactInt 1.5.3, FGA 1.3.1, GAPDoc 1.5.1, IO 4.4.6, 
            IRREDSOL 1.3.1, Polycyclic 2.11, SpinSym 1.5, TomLib 1.2.6 

Gone from the packages loaded by default, due to dependencies, are: AClib, Cryst, CrystCat, LAGUNA, Polenta, RadiRoot, ResClasses, Sophus and Utils.

[olexandr-konovalov]

The failure in the test of manual examples is harmless - they could be updated to the new output, However, not all chapter have been tested. I've submitted issue #1138 to suggest that this test should run through all chapters.

[olexandr-konovalov]

After the last commit (removing Alnuth) I've run the test of manual examples locally, and indeed the diffs are harmless (see below), and manual examples could be updated. With

default:= [ "autpgrp", "ctbllib", "factint", "fga", "irredsol", "tomlib" ],

the list of packages loaded at startup is:

 Packages:   AtlasRep 1.5.1, AutPGrp 1.6, Browse 1.8.6, CRISP 1.4.4, CTblLib 1.2.2, 
             FactInt 1.5.3, FGA 1.3.1, GAPDoc 1.5.1, IO 4.4.6, IRREDSOL 1.3.1, 
             SpinSym 1.5, TomLib 1.2.6

If we agree about that, I will make one more commit to adjust manual examples, and this will be ready for merge.

########> Diff in [ "./../../lib/oper.g", 468, 488 ]
# Input is:
Size( g );
# Expected output:
#I  immediate: IsPerfectGroup
#I  immediate: IsNonTrivial
#I  immediate: Size
#I  immediate: IsFreeAbelian
#I  immediate: IsTorsionFree
#I  immediate: IsNonTrivial
#I  immediate: IsPerfectGroup
#I  immediate: GeneralizedPcgs
#I  immediate: IsEmpty
6
# But found:
#I  immediate: IsPerfectGroup
#I  immediate: IsNonTrivial
#I  immediate: Size
#I  immediate: IsNonTrivial
#I  immediate: IsPerfectGroup
#I  immediate: GeneralizedPcgs
#I  immediate: IsEmpty
6
########
########> Diff in [ "./../../lib/object.gd", 708, 744 ]
# Input is:
KnownPropertiesOfObject(g);
# Expected output:
[ "IsFinite", "CanEasilyCompareElements", "CanEasilySortElements", 
  "IsDuplicateFree", "IsGeneratorsOfMagmaWithInverses", 
  "IsAssociative", "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose", "IsNilpotentByFinite" ]
# But found:
[ "IsFinite", "CanEasilyCompareElements", "CanEasilySortElements", 
  "IsDuplicateFree", "IsGeneratorsOfMagmaWithInverses", 
  "IsAssociative", "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose" ]
########
########> Diff in [ "./../../lib/object.gd", 708, 744 ]
# Input is:
KnownPropertiesOfObject(g);
# Expected output:
[ "IsEmpty", "IsTrivial", "IsNonTrivial", "IsFinite", 
  "CanEasilyCompareElements", "CanEasilySortElements", 
  "IsDuplicateFree", "IsGeneratorsOfMagmaWithInverses", 
  "IsAssociative", "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose", "IsPerfectGroup", "IsSolvableGroup", 
  "IsPolycyclicGroup", "IsNilpotentByFinite", "IsTorsionFree", 
  "IsFreeAbelian" ]
# But found:
[ "IsEmpty", "IsTrivial", "IsNonTrivial", "IsFinite", 
  "CanEasilyCompareElements", "CanEasilySortElements", 
  "IsDuplicateFree", "IsGeneratorsOfMagmaWithInverses", 
  "IsAssociative", "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose", "IsPerfectGroup", "IsSolvableGroup", 
  "IsPolycyclicGroup" ]
########
########> Diff in [ "./../../lib/object.gd", 708, 744 ]
# Input is:
KnownTruePropertiesOfObject(g);
# Expected output:
[ "IsNonTrivial", "IsFinite", "CanEasilyCompareElements", 
  "CanEasilySortElements", "IsDuplicateFree", 
  "IsGeneratorsOfMagmaWithInverses", "IsAssociative", 
  "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose", "IsSolvableGroup", "IsPolycyclicGroup", 
  "IsNilpotentByFinite" ]
# But found:
[ "IsNonTrivial", "IsFinite", "CanEasilyCompareElements", 
  "CanEasilySortElements", "IsDuplicateFree", 
  "IsGeneratorsOfMagmaWithInverses", "IsAssociative", 
  "IsGeneratorsOfSemigroup", "IsSimpleSemigroup", 
  "IsRegularSemigroup", "IsInverseSemigroup", 
  "IsCompletelyRegularSemigroup", "IsCompletelySimpleSemigroup", 
  "IsGroupAsSemigroup", "IsMonoidAsSemigroup", "IsOrthodoxSemigroup", 
  "IsFinitelyGeneratedGroup", "IsSubsetLocallyFiniteGroup", 
  "KnowsHowToDecompose", "IsSolvableGroup", "IsPolycyclicGroup" ]
########

[Stefan-Kohl]

As far as I recall, CRISP has badly interfered for several times with other code.
Perhaps the most relevant reason in favor of loading this package by default is (citation from the abstract): "an improved method to compute the set of all normal subgroups of a finite soluble group, its characteristic subgroups, minimal normal subgroups and the socle and p-socles for given primes p. "
I don't know whether these methods are sufficiently better than the Library methods to justify loading CRISP by default or not.

The SpinSym package (a suggested package of CTblLib) is quite special-purpose. -- I wouldn't consider loading such special-purpose package by default.

[olexandr-konovalov]

Just to emphasise that there are no CRISP and SpinSym in this list:

default:= [ "autpgrp", "ctbllib", "factint", "fga", "irredsol", "tomlib" ],

CRISP is suggested by irredsol and SpinSym by CTblLib, so if default is as above, they have to stay.

[Stefan-Kohl]

What about loading only needed packages, but not suggested packages when packages are loaded by default? -- The rule that also suggested packages are loaded basically discourages suggesting packages in packages loaded by default (or else, the number of packages loaded by default could explode for basically no reason once a new version of a package loaded by default is released).

[olexandr-konovalov]

There is no easy way of doing this: OnlyNeeded option does not work well if one decides to load suggested package later. It is used only in tests to look for undeclared dependencies etc.

The scope of this PR is just to change the value of the default list as shown in https://github.com/gap-system/gap/pull/1135/files, and move further from its current value which is a legacy from 4.4.12.

[Stefan-Kohl]

In that case I'd suggest to remove CTblLib and irredsol from the list. -- In the end, CTblLib is interesting only for people who work on representation theory of finite groups, and irredsol is only interesting for people who are working on finite solvable linear groups -- both of these being quite narrow subtopics within the broad range of possible applications of GAP. In the same spirit, I'd then suggest to remove tomlib from the list as well. I see loading the libraries of character tables and of tables of marks by default mainly as a relic from history. This leaves only AutPGrp, FactInt and FGA, and none of these 3 packages has any dependencies besides GAPDoc and GAP itself.

[stevelinton]

I think Stefan may well be right -- ultimately we should only load packaged needed by GAP and packages that contain better methods for library operations such as the three listed. We would need to think very carefully about how to manage the transition though -- we don't want people suddenly finding familiar and long-established functionality missing and not knowing how to restore it.

So as a first step. making sure that tests pass with a minimal set of packages is good, but we shoyld have a very clear plan for any change in the release

[olexandr-konovalov]

• Grepping for IsPackageMarkedForLoading in lib and grp points on using atlasrep, FactInt and tomlib, so I think those three packages should stay, as well as autpgrp and FGA.

• We can now check what happens with irredsol being excluded too

• With AtlasRep there, excluding CTblLib does not give any practical effect, because AtlasRep suggests browse, ctbllib, tomlib and IO.

• Also, excluding CTblLib will force us to update some manual examples to ensure that it is loaded, and I think that would be one of the common cases of not being able to find something familiar.

But perhaps setting default to [ "atlasrep", "autpgrp", "factint", "fga", "tomlib" ] will reflect the logic of forming this list better. With these five packages, the "dependencies closure" triggers loading the following set of packages:

AtlasRep 1.5.1, AutPGrp 1.6, Browse 1.8.6, CTblLib 1.2.2, FactInt 1.5.3, 
FGA 1.3.1, GAPDoc 1.5.1, IO 4.4.6, SpinSym 1.5, TomLib 1.2.6

I've committed this change to see how CI tests will respond.

[ChrisJefferson]

We can separate the problems users will get with missing packages into two sets:

1. Functions gets slower due to missing overloads -- hard to fix, let's ignore that for a moment.

2. Functions missing.

This second one is much easier to fix, we could adjust the normal GAP error (for example)

gap> ReadLineByLineProfile;
Error, Variable: 'ReadLineByLineProfile' must have a value
not in any function at *stdin*:47

To something like (just a first attempt)

gap> ReadLineByLineProfile;
Error, 'ReadLineByLineProfile' must have a value
'ReadLineByLineProfile' is provided in the 'Profiling' package.
not in any function at *stdin*:47

This would be very easy in principle, as we already have this stuff in the manual! The only thing stopping me from doing it is I don't know how to tell if something in the manual is actually the name of a function/method, or just a random documented word.

[olexandr-konovalov]

I've run a test of manual examples locally to test all chapters (Travis currently terminates the tests after the first chapter with diffs, see #1138). It went OK. Besides the same diffs as above, the only new one is

########> Diff in [ "./../../lib/grp.gd", 1836, 1840 ]
# Input is:
g:=SymmetricGroup(4);;NormalSubgroups(g);
# Expected output:
[ Sym( [ 1 .. 4 ] ), Group([ (2,4,3), (1,4)(2,3), (1,3)(2,4) ]), 
  Group([ (1,4)(2,3), (1,3)(2,4) ]), Group(()) ]
# But found:
[ Group(()), Group([ (1,4)(2,3), (1,3)(2,4) ]), Group([ (2,4,3), (1,4)
  (2,3), (1,3)(2,4) ]), Sym( [ 1 .. 4 ] ) ]
########

because for finite soluble groups CRISP provides an efficient method to compute NormalSubgroups and now it falls back to the library one. The ordering is not a problem - there is no promise of the ordering of subgroups, so this should be ok. But the performance is. For example, in the current master branch

gap> G:=DihedralGroup(2^16);;NormalSubgroups(G);;time;

takes 83 ms with CRISP and 6635 ms without it. So perhaps CRISP should stay too. I am not aware of any present problems of CRISP, and it is now hosted at https://github.com/bh11/crisp, so hopefully should any problems arise they may be resolved efficiently (I am more worried about other packages from the default list not being visible at https://gap-packages.github.io/).

[Stefan-Kohl]

So CRISP's methods may be notably faster, at least in special situations. On the other hand, CRISP contains much very special-purpose code. -- Wouldn't this suggest either to split up CRISP into 2 packages, one of them containing the methods improving Library functionality (to be loaded by default) and the other one containing the special-purpose functionality (not to be loaded by default) -- or to move the methods which improve Library functionality into the Library?

[hulpke]

Concerning ctbllib and tomlib: There are able documented examples of GAP that use the character table library without explicitly loading it. Many people consider this a core functionality of GAP, and while it is not fundamentally hard to load a package, this is another hurdle for a beginner.

tomlib is fundamentally used by the subgroup lattice and maximal subgroups calculations to obtain subgroups of simple groups -- without having it available such calculations default to the much slower cyclic extension method. Calculations will still run but some tests will be notably slower.

I thus believe very strongly that both packages should remain autoload.

[ChrisJefferson]

Practically, what is the advantage for users of us removing packages from the current autoload?

If some package only improves the user's experience when using GAP, then not autoloading it seems silly.

[olexandr-konovalov]

@ChrisJefferson but then this package should be listed in the default list which the precise mechanism for that, and we should have a record why it's there. Presently default list is

[ "autpgrp", "alnuth", "crisp", "ctbllib", "factint", "fga", 		
  "irredsol", "laguna", "polenta", "polycyclic", "resclasses", 		
  "sophus", "tomlib" ],

which is a relic from times when package authors were self-nominating packages to be autoloaded. Now we want to revise it properly.

[Stefan-Kohl]

@hulpke Well -- there are so many possible applications of GAP for which loading a character table library by default looks like loading a flight simulator in an office software package ... .
Having to load packages is really not an issue for a user -- of course provided that it is clearly communicated which package to load and how to do this, whenever one tries to use functionality of a package which is installed, but not loaded. One could even think about automatically loading a package once any of its documented functions etc. is called / used for the first time.

[frankluebeck]

I do not support to merge this pull request.

In general I agree with the goal to be able to start GAP with as little functionality as possible. (Optimally, just with the basic builtin data types, the language and the tools to load further functionality. For this it would be good to move almost everything in the library into packages.)

Therefore, it is a sensible step to think about reducing the number of packages loaded by default.

The current list of default packages was decided for historical reasons. These are the packages which had set the AutoLoad flag when this was still possible, and the default list ensured backward compatibility with older versions of GAP. Note that every user can easily overwrite the default list via a user preference.

What are reasons for not loading a package by default?

• it is only needed by some GAP users and not by many/most others
• loading it although it is not needed
  • costs startup time
  • costs memory in RAM and workspaces
  • fills the global namespace or lists of methods
• users who make good use of the package may never find out that they do so

What are reasons for loading a package by default?

• the package overwrites library functionality (or other packages functionality?) by better code
• the package adds new functionality to GAP
• users can make good use of the packages functionality without ever becoming aware of this

The last point is the same in both cases but with opposite interpretations.

Just the first point for loading a package is somewhat random because it has also mainly historic reasons what functionality is in the library and what is in packages; in my opinion the first and second point for loading a package by default should be considered together.

We have to decide if we rate higher the reasons for and against loading packages by default.

If we rate the points for not loading a package higher, then I think we should empty the list of default packages completely; no package functionality is useful for everyone. In this case we have to communicate this change very well to the users (because previous default functionality may be missing or is less efficient). It essentially means that we urge every user to think about her/his personal list of sensible default packages and to configure this in their gap.ini file. Maybe some users will be annoyed by this. On the other hand users become much better aware which packages they (want to) use.

If we rate the points for loading a package higher, we should completely forget the (somehow accidental) list of current default packages. In this case every available package should be considered and decided if it enhances some functionality or adds new functionality for some GAP users. The result of this consideration is probably a default list that is close to the effect of LoadAllPackages.

My personal preference would be towards an emtpy list of default packages. But before putting this into practice we could think about possibilities to help users to find out about available functionality. For example, GAP could know a list of hints; if a function is called that is not yet defined but available in a package, GAP could give a hint; or if an operation is called for the first time and GAP knows that some not loaded packages have additional methods a hint could be given.

The currently suggested list of default packages in this pull request looks pretty random to me and does not make sense. The gain of having just a few packages less in the default list is too small to bother all users with lengthy explanations about the change (and I really cannot see convincing reasons why the current list is more sensible than any other subset of packages).

[olexandr-konovalov]

@frankluebeck thank you for detailed feedback. The list [ "atlasrep", "autpgrp", "factint", "fga", "tomlib" ] does not look random to me - atlasrep, factint and tomlib are used by the library, an the two other fit under "overwrites library functionality by better code" for specific cases (free groups; p-groups). I am going to document that better in the next version of this pull request.

But I think that you're right that we need to think about better ways to help users to discover functionality in packages first. The idea of @ChrisJefferson to tweak Error, 'SomeThing' must have a value message is nice, if going that way then we need to ensure that all package manuals are loaded, and find some heuristics to avoid misleading suggestions...

[olexandr-konovalov]

Update: this is not planned for GAP 4.9.0 and may be revisited later if there is a progress with #1173.