Conversation
|
Thanks for working on this! So the new Once major concern I have is how we plan to transition packages which install methods for So I think we need to go the other way around: we first must work with package authors to give them the chance to adapt and release their packages, then we can move forward. Here is one way this could be done:
That's how I've done similar migrations in the past. It's a slow and tiresome process, Ia m afraid... Here is a pull request from 2019 by me for FGA which still has not been merged, much less released: chsievers/fga#7. On the upside, all the other packages have a fairly good track record for releases (well perhaps except for Anyway: because this is so tedious, it's best to not do this too often. So here one may wonder if there aren't other methods that need similar treatment and that should be dealt with in one go. E.g. I wonder whether we also need |
|
There were 3 files where it appeared that after a call to PreImagesRepresentativeNC a test for the return of fail followed immediately: alghom.gi, ghomperm.gi, mapphomo.gi. |
|
I am very perplexed by some methods in mapprep.gi which are full of tests for "im = fail". Should these be methods for the non-NC PreImagesRepresentative? |
|
@fingolfin Sorry if I'm slow on the uptake, but I do not understand what will be broken. As it stands at present PreImagesRepresentative just calls PreImagesRepresentativeNC (without any test). So, it seems to me, everything should work as before, and changes to packages can come later. |
|
PreImages produces the same error as PreImagesRepresentative, so the same procedure applies. |
|
Further details of the use by packages of one or more of PreImages, PreImagesElm, PreImagesSet and PreImagesRepresentative. |
|
When nothing happens for weeks one can forget exactly what has been done. |
| natural numbers.) We can now form images and preimages under the natural | ||
| homomorphism. The set of preimages of an element under <C>hom</C> is a coset | ||
| modulo <C>elab</C>. | ||
| We use the function <Ref Func="PreImages" BookName="ref"/> here because |
There was a problem hiding this comment.
I think this can just be left as PreImages, in general in documentation we don't encourage people to use NC methods of functions. Of course at the moment it doesn't make any difference.
There was a problem hiding this comment.
@ChrisJefferson Is this rule formulated somewhere in the documentation?
(If we do not want to encourage using NC variants in the documentation then this pull request should not change examples from the manual to use PreImagesRepresentativeNC instead of PreImagesRepresentative.)
I would agree that the NC variants are mainly intended to be used inside functions where it is clear from the context that the tests are not necessary. On the other hand, one can argue that the same holds for an interactive GAP session, for example, if one wants to compute a preimage of an element that was just constructed as an element of the range of the mapping in question.
There was a problem hiding this comment.
@ThomasBreuer Looking around when defining function we often define the NC and non-NC version at the same time, but when we reference we often only reference one.
I do think if we are mentioning PreImagesNC we should say what the difference is -- I know someone could follow the links to PreImages and PreImagesNC, but it is a bit strange here to see both mentioned.
In general it is probably worth thinking about if we want to use PreImagesRepresentative or PreImagesRepresentativeNC in most examples. I am not sure in general how much slower PreImagesRepresentative is going to be, so which we should be encouraging as a default -- but I would hope PreImagesRepresentative, because it is safer for users who are beginners.
There was a problem hiding this comment.
@ChrisJefferson Thanks for your thoughts.
Currently I find one statement about NC variants in the Tutorial Manual, which talks about Sub<something> vs. Sub<something>NC, and one statement about Sub<struct>NC in the Reference Manual Section "Constructing Subdomains"
I think there should be a general statement about Func vs. FuncNC (in the Tutorial as well as in the Reference Manual), saying that the latter omits some argument checks and that the documentation of the individual functions will explain which checks are omitted.
Some example that illustrates the difference should be shown in the Tutorial: When one creates a subgroup of a group then calling SubgroupNC instead of Subgroup will avoid membership tests for the subgroup generators; in the case of a permutation group, this may mean that the computation of a stabilizer chain for the big group can be avoided, which would hopefully be not expensive; for a matrix group such as the Baby Monster, calling SubgroupNC will be the only reasonable way to create a subgroup, since asking for membership will be in general hopeless.
In the Reference Manual, Func and FuncNC should be documented in the same ManSection, and then cross-references need to mention only the non-NC variant.
Concerning the use of NC variants in manual examples, I see two possible strategies. Either we use only the non-NC variants, or we use the NC variants in all those cases where the context admits this (perhaps with a textual comment why the NC variant may be called in examples in the Tutorial and in those cases where this is not obvious).
I would expect that the differences in runtime are negligible in manual examples, and if this is not the case for some example then this example deserves a comment about this fact.
There was a problem hiding this comment.
I agree that it is not a good idea to mention NC variants in the tutorial manual, and will make the necessary changes. The more general comments are very interesting but are beyond the scope of this PR.
There was a problem hiding this comment.
Removed NC references in doc/tut/group.xml and doc/tut/algvspc.xml. Had problems trying to push these - no idea why - so used --force. Hope that does not cause any problems.
| end; | ||
|
|
||
| InstallMethod( PreImagesRepresentativeNC, | ||
| InstallMethod( PreImagesRepresentative, |
There was a problem hiding this comment.
Is the idea here that these methods already do all appropriate checking, so we don't need to attach "NC" to them?
There was a problem hiding this comment.
The reason I ask is that there are two possible issues this raises:
-
We should probably have tests which make sure that make sure this is a "non-NC" method.
-
Installing a method for PreImagesRepresentative, but not for PreImagesRepresentativeNC, means there could be cases where PreImageRep works, while PreImageRepNC doesn't.
I realise that these two things are currently issues, as PreImageRep and PreImageRepNC are the same operation with different names, but it's probably worth having a long term plan for handling this before installing methods for both PreImagesRep and PreImagesRepNC in different places.
There was a problem hiding this comment.
Surely all methods, such as in alghom.gi, should be for PreImagesRepresentativeNC, PreImagesSetNC and PreImagesElmNC. The real question is whether calls to such methods should be to the NC or non-NC versions. Since, up until now, there has been no test that element elm in in the image, it is reasonable to assume that the authors of such calls were confident that this was the case. That is why all the calls have been changed to the NC versions. In due course, once packages have had the chance to make changes, non-NC methods, making a check, will be added. Once that is done the status of these calls could be reviewed.
|
I started writing this in a comment, but it got too long, so I'll pop it here. We could consider adding something like: This could, in principle, then be used in other places, for just having a quick search for |
cdwensley
changed the title
|
@fingolfin Sorry to be bothering you again. With my system more or less back to where it was, I would like to restart work on this PR. However, the branch of gapdev I was using has been lost. I have used "git fetch upstream refs/pull/5073/head:preimrep" to restore it; rebased on yesterday's master; and dealt with all the resulting diffs. How to push this to 5073? If I "git push origin preimrep" I am asked to create a new PR. If I "git push upstream preimrep" the request is rejected (probably not a fast-forward). Is there a simple solution, or is a new PR a good idea, with a comment here that the PR has been updated? There are a large number of suggestions here that should not be lost. |
|
No progress on this since last February. Restarting activity today with PR#98 for Wedderga. |
|
Progress report: |
|
In order to fix conflicts in 22 lib/.gi and lib/grpnice.gd I have made copies from master; changed all the PreImages to nPreImages*.NC; and then committed these edited copies. The result is that that checks fail because of changes in some of the corresponding lib/*.gd files. These can be fixed in the same way by making copies from master and, so far, grplatt.gd and gpr.gd have been processed in this way. |
|
And now fitfreee.gd. It's a pity tests fail after finding the first undefined variable, rather than looking for more. |
|
I'm thinking that it might be best to close this PR and start again with a branch of the current gapdev? |
|
There were so many conflicts between files edited in this PR and changes made in master that it seemed sensible to rebase the PR on the current master and make sure all the previous changes to ...NC were made. This has now been done, which will hopefully allow most of the checks to pass. |
|
PR #58 for FR merged 11/01/2024and included in release 2.4.13. |
|
@cdwensley there was a merge conflict which I took the liberty of resolving. It would be great to get this finally done. Thank you for not dropping the torch on this! |
|
Perhaps to keep track of progress, we could take the list from #5073 and copy it in slightly edited form to the issue description; I would turn it into a task list, i.e. which renders as
and one can "tick the boxes". Then for each package, if there is a relevant issue or PR, I'd add a link to that. This way one can quickly see what's done already, what is in progress, and what is incomplete. |
|
The plan was to get those packages with methods installed for one of PreImages, PreImagesElm, PreImagesSet and PreImagesRepresentative to prepare for this merge. Here is my list of such packages:
A number of the other distributed packages call one of more of these functions a handful of times in their library, but it seems unnecessary for them to make any adjustments at this stage. If @fingolfin is able to turn this into a task list, that can only be helpful. |
|
@cdwensley turned it into a list, and the PR refs into links... please verify I set the checkboxes right (and otherwise just adjust them). Some additional remarks:
|
|
@fingolfin: how you changed the PR references in the list above into links I cannot imagine, but they all appear to be correct. |
* add checks to `NewVector`, `NewMatrix` for the GF(2) and 8 bit repres.
in order to catch `fail` results
* add a `String` method for rings `Integers mod n`
* fix the default `OneMutable` method for `IsMatrixObj`
Test that the input is square, as for list-of-lists matrices.
* minor fixes for `IsPlistVectorRep`
- check the input list for `IsPlistRep` in `MakeIsPlistVectorRep`,
let `NewVector` turn the input to `IsPlistRep` if necessary
- forbid assignments into a `IsPlistVectorRep` vector beyond its length
* fix `SolutionMatDestructive`
Conceptually, the solution must be in the same representation
as the given vector.
(Let us see which other bugs will come to the surface now.)
* fix `TestPositionNonZeroInRow`
Use a mirror is more resilient and often faster, as one close to you is selected.
In each case, the new code is in fact slightly faster, unless of course IdGroup already happens to be set for the input group.
|
Have successfully rebuilt my version of this branch. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5073 +/- ##
==========================================
- Coverage 78.67% 78.67% -0.01%
==========================================
Files 683 680 -3
Lines 292943 292592 -351
Branches 8658 8578 -80
==========================================
- Hits 230486 230183 -303
+ Misses 60638 60610 -28
+ Partials 1819 1799 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…a new feature for matrix groups (#6232) - add a description of the cache `FULLGLNICOCACHE` in the code; it is used only by `NicomorphismFFMatGroupOnFullSpace` - change the keys of `FULLGLNICOCACHE`: take the field of the matrix entries into account (not only its size), take the `ConstructingFilter` of the matrices into account - in `NicomorphismFFMatGroupOnFullSpace`, deal with special cases of matrices in `IsBlockMatrixRep` (which have no `ConstructingFilter`) and in `Is8BitMatrixRep` in the situation that the group itself lives over `GF(2)` - in order to admit the action of `IsMatrixObj` matrices on vectors supported by GAP's `Enumerator`s of vector spaces, add a `\^` method that `Unpack`s the matrix object (Eventually we want to avoid this overhead, but then we need `Enumerator`s consisting of vector objects corresponding to the matrix objects.) - add `Matrix` calls in the function that computes preimages under an action homomorphism with `Source` a matrix group, in order to support matrix group elements of prescribed kinds of `IsMatrixObj`. - add tests for the new supported situations (most of the changes were actually forced by already available tests) * add `\in` methods for matrix object and (`GL` or `SL`) * avoid some `Unpack` hacks - introduce `NormedRowVectors_internal( F, base )`, and `AsListOfFreeLeftModule_internal( F, base, zero )`, in order to admit `IsVectorObj`s in `base` without supporting the whole vector space machinery for these objects - introduce `ExternalSet( G )` for matrix groups `G`, meaning the natural `G`-set (an undocumented method for permutation groups `G` was already available) - change `NicomorphismFFMatGroupOnFullSpace` such that we can act with the `IsMatrixObj` matrices on their `IsVectorObj` vectors, without unpacking the matrices - support `IsMatrixObj` matrices in the `Random` methods for GL and SL * adjust also `IsProjectiveActionHomomorphism` methods * add `RandomMatrix`, `RandomInvertibleMatrix` and fix the `RankMat` method for `IsPlistMatrixRep`
- remove a bunch of SmallGroup and IdGroup uses in tst files - remove tst/testbugfix/2005-05-03-t00070.tst (an equivalent test was added to the smallgrp package recently) - modify some .tst file to run parts only if certain packages are available, via `#@if IsPackageMarkedForLoading(...)` - don't load `ctbllib` (and all its dependencies) in `ctblmaps.tst`
|
Lots more rebasing going on. |
|
Assuming all necessary packages have added "NC" to PreImagesRepresentative, this moves to the next stage with edits in lib/mapping.g* by adding the test to PreImagesRepresentative that the element is in the image. Have also edited out the "NC"s in tst/testinstall/*.tst. These all appear to pass. |
|
Have now added methods for PreImages, PreImagesElm, PreImagesSet and PreImagesRepresentative, and removed the "NC" from their calls in tst/testinstall/*.tst. @fingolfin - was this what I was supposed to do? |
|
Review of what has happened so far: |
|
Attempted to do another "git rebase master" but gave up after fixing about 30 conflicts because there seemed to be dozens more. Have no idea how to proceed with this PR. Is the best plan to start a new PR, making the same changes in the main library, and then close the old one? |
|
This PR has been superceded by a second version: PR #6409. |
17 participants
This aims to start work on the process outlined in issue #4809. The operation PreImagesRepresentative has been renamed PreImagesRepresentativeNC throughout the library, many doc files, and some tests. A new PreImagesRepresentative has been introduced which just, for now, calls PreImagesRepresentativeNC. If this change is included in 4.12.1 then package authors can be given the option of converting their calls of this operation. After that a test can be introduced to check that the element is in the range of the map. Further changes may be recommended, e.g. a new PreImageElmNC.
Text for release notes
Too early to write this down.