-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load PrimGrp, SmallGrp and TransGrp packages if available #1714
Load PrimGrp, SmallGrp and TransGrp packages if available #1714
Conversation
hpcgap/lib/package.gi
Outdated
@@ -1824,6 +1824,19 @@ BindGlobal( "BANNER", false ); | |||
|
|||
GAPInfo.delayedImplementationParts:= []; | |||
|
|||
# Ensure GAP loads PrimGrp, SmallGrp and TransGrp packages | |||
# To be migrared to lib/system.g after the replacement of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: migrated
hpcgap/lib/read.g
Outdated
## callable, even if the library is unavailable. | ||
if TestPackageAvailability("PrimGrp")=fail then | ||
InstallGlobalFunction(PrimitiveGroupsAvailable,deg->false); | ||
fi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be something like this for smallgrp, too? @markuspf is there? (Not on my computer, so I can't check right now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already there, above those lines:
if TestPackageAvailability("smallgrp")=fail then
ReadSmall( "readsml.g","small groups" );
fi;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its currently called SMALL_AVAILABLE
and t akes as an argument a number and either returns a record or fail
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-konovalov no, that's something else
@markuspf right, I know about that one, but I was thinking about something more user friendly and actually documented (SMALL_AVAILABLE
is an internal function). I've submit this as gap-packages/smallgrp#20
hpcgap/lib/galois.gi
Outdated
@@ -546,7 +546,7 @@ local f,n,i,sh,fu,ps,pps,ind,keineu,ba,bk,j,k,a,anz,pm, | |||
Info(InfoPerformance,2,"Using Transitive Groups Library"); | |||
f:=arg[1]; | |||
f:=f/LeadingCoefficient(f); | |||
if not(IsIrreducible(f)) then | |||
if not(IsIrreducibleRingElement(f)) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are changing this anyway, perhaps remove the superfluous parenthesis?
Codecov Report
@@ Coverage Diff @@
## master #1714 +/- ##
===========================================
- Coverage 63.43% 49.28% -14.16%
===========================================
Files 1000 459 -541
Lines 322760 239320 -83440
Branches 13015 10724 -2291
===========================================
- Hits 204757 117959 -86798
- Misses 115361 118627 +3266
- Partials 2642 2734 +92
|
HPC-GAP fails with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me.
63b4a5a
to
cd19dd6
Compare
Hm, unlike |
lib/read4.g
Outdated
|
||
if not IsBound(HPCGAP) then | ||
# Galois group identification relies on the list of transitive groups | ||
# (in HPC-GAP these file are read in read.g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why is this different between GAP and HPC-GAP? The comment should explain that (which is the difficult, non-trivial part -- in contrast figuring out where GAP vs. HPC-GAP loads something can be figured with a simple "grep" in a few seconds)
And independently of that: Why does regular GAP put this into read4.g
? I'd expect this to be in read3.g
and read5.g
, for the .gd and .gi respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fingolfin: Because we want to please both installations with and without new packages. Starting GAP with -D
option, one can see that the relative order in which these files are read is different. If there is no transgrp package, it is
#I READ_GAP_ROOT: loading 'trans/trans.gd' as GAP file
#I READ_GAP_ROOT: loading 'trans/trans.grp' as GAP file
#I READ_GAP_ROOT: loading 'trans/trans.gi' as GAP file
#I READ_GAP_ROOT: loading 'lib/galois.gd' as GAP file
#I READ_GAP_ROOT: loading 'lib/galois.gi' as GAP file
With it we have
#I READ_GAP_ROOT: loading 'lib/trans.gd' as GAP file
#I READ_GAP_ROOT: loading 'lib/galois.gd' as GAP file
#I Read( "/Users/alexk/GITREPS/gap/pkg/transgrp/lib/trans.grp" )
#I Read( "/Users/alexk/GITREPS/gap/pkg/transgrp/lib/trans.gi" )
#I READ_GAP_ROOT: loading 'lib/galois.gi' as GAP file
Furthermore, TRANSREGION
is defined in hpcgap/trans/trans.gd - when the package is present, this will not be read, so I define it in hpcgap/lib/read.g
.
I was testing this with packages and without package, and found this "equilibrium". It works now while we need to cater for both setups. As soon as we will go to the next stage and will remove trans
and hpcgap/trans
, I will look again and will try to converge handling of this in GAP and HPC-GAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-konovalov I still don't understand. You described the effect, telling me that the load order is different between GAP and HPC-GAP -- and, well, of course it is, as e.g. the ReadLib
calls in a different order. But that still doesn't give me a logical reason for why we do it like that.
My POV is this: Either there are strong reasons why it must be different, then we should document them in comments quite clearly. Or else, there are purely technical obstacles which we can and should remove, to make it possible to align this code.
The TRANSREGION
sounds like the latter to me. Indeed, this is completely missing from the transgrp
package -- so I guess before we can fix it, we must make transgrp
HPC-GAP ready.
I'll look into that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fingolfin it's not only that the load order is different between GAP and HPC-GAP - it is also different dependently on whether the data library is a package or directory.
I have been trying to resolve all problems without making any changes in transgrp, assuming that those can be made eventually - transgrp is not hosted under gap-packages and has a longer release cycle. If you could look at that, please do, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But transgrp
does not contain any HPC-GAP specific changes. So with this PR, support for transitive groups in HPC-GAP will be completely broken, no? And probably also for smallgrp
, unless that contains HPC-GAP specific changes (I didn't see any on a quick glance just now... @markuspf ?)
So, for now it might be best to leave out the LoadPackage
bits for HPC-GAP, until those packages contain the HPC-GAP specific changes, no?
hpcgap/lib/read.g
Outdated
## | ||
## Assign TransitiveGroupsAvailable to a dummy function to make it | ||
## callable, even if the library is unavailable. | ||
if TestPackageAvailability("TransGrp")=fail then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please change this to "transgrp"
, to match the other places (same for PrimgGrp
and SmallGrp
below). Or, if you prefer, change all other places to use TransGrp
. Just as long as it is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I think because LoadPackage and friends is case-insensitive, lowercase everywhere for all packages would be consistent (I do not use uppercase when requesting a package, and I think the same applies to most of users).
The canonical NAME of the package is the other thing. Here authors may combine upper and lower case as they wish. My preferred spelling would be mixed case, like PrimGrp, SmallGrp, TransGrp in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like the packages to use that spelling for their canonical name, then feel free to submit pull requests to change their PackageInfo.g
files accordingly -- right now, though, transgrp and smallgrp use an all-lowercase spelling, only PrimGrp does not.
44cc775
to
837d829
Compare
If any of these packages is not available, GAP will read corresponding directory (`prim`, `small` or `trans`). Also replaced the call of `IsIrreducible` in `lib/galois.g` by `IsIrreducibleRingElement` to allow reading it earlier than `overload.g`, and moved definition of `TRANSREGION` out of `trans` directory to make it working both with `trans` and TransGrp package.
837d829
to
3af6942
Compare
This has been rebased. Tests without and with prim and trans should be passing (hopefully).
As for whether It is defined in When |
I can update merged packages archive to rollback to the previous |
I will now rollback to the stable version of NormalizInterface while @sebasguts will prepare a new one. This should make tests pass. |
Tests are now passing. But I've downloaded and installed smallgrp package, and in HPC-GAP I have this:
Will investigate tomorrow. |
The third package - SmallGrp - has now been picked up for the redistribution, so this PR is now fully tested and we can merge it now. Next step would be to instruct everyone to download and install the three new packages - PrimGrp, TransGrp, SmallGrp (individually or via After that, when #1650 will be merged, GAP will not start without the three packages, so everyone who will not switch by that time, would have to download and install them. |
PR gap-system#1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
PR gap-system#1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
PR gap-system#1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
PR gap-system#1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
PR gap-system#1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
PR #1714 added some code needed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise. This code is not needed any more, since we remove prim, trans, and small directories. Also make PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing.
This is a modification of some changes attempted in #1650 (part of the plan attempted in #1650 in order to make the transition less disruptive).
If any of these packages is not available, GAP will read corresponding directory (
prim
,small
ortrans
).Dependency on
TRANSGrp
defined in the package required readinglib/galois.g*
files in another place (inread4.g
). In its turn, they requireIsIrreducible
which is defined inoverload.g
and delegates toIsIrreducibleRingElement
, hence the latter was used.Since the CI scripts are note yet using
packages-master.tar.gz
, this will now be tested for the setting in which none of the three new packages is available.The next step would be to switch to
packages-master.tar.gz
and run CI tests with two of the packages - PrimGrp and TransGrp.When this will be merged, we will be able to tell developers to get the new packages in question, either installing them individually or by removing
pkg
directory and callingmake bootstrap-pkg-full
to get the latest collection of GAP packages for the master branch.After that, assuming that developers obtained relevant packages, we may proceed with next steps (could be done for one package at a time):
GAPInfo.Dependencies.NeededOtherPackages
.