Document and enforce that "nice monomorphisms" must have IsInjective set (otherwise an infinite recursion can happen)
#5029
Conversation
ThomasBreuer
added
kind: bug
This addresses issue gap-system#5020. We check whether the map that shall be set as nice monomorphism has already the `IsInjective` flag, and signal an error if not.
`MultiActionsHomomorphism` does not know about injectivity, but since it is called with the `SeedFaithfulAction` data, we can safely set the flag for the returned map.
ThomasBreuer
force-pushed
the
TB_SetNiceMonomorphism
branch
from
6edbf24
to
0a2378f
Compare
| only if this map knows that it is injective. | ||
| In the above example, this is the case because the map is created as the | ||
| composition of two maps (a nice monomorphism and an isomorphism) | ||
| which know that they are injective. |
There was a problem hiding this comment.
Perhaps this and/or the other new text could mention that one can ensure this is the case by either invoking IsInjective(map) (safe but potentially slow) or SetIsInjective(map, true) (fast but the caller has to make sure this is correct) ?
lib/grpnice.gi
Outdated
| ## monomorphism that it is one. | ||
| ## We install a special setter method because we have to make sure that only | ||
| ## injective maps get stored as nice monomorphisms. | ||
| ## More precisely, we the stored map must know that it is injective, |
There was a problem hiding this comment.
| ## More precisely, we the stored map must know that it is injective, | |
| ## More precisely, the stored map must know that it is injective, |
lib/grpnice.gi
Outdated
| ## (The maps computed by 'NiceMonomorphism' have the 'IsInjective' flag, | ||
| ## the test affects only those maps that shall be set by hand as | ||
| ## nice monomorphisms.) |
There was a problem hiding this comment.
Strictly speaking this is just the case for all the current such methods. Anyone adding more such methods in the future should also be aware of it, I think? So perhaps the sentence here should either be changed to acknowledge that -- or just remove it? I don't think it is all that useful: we want to enforce an invariant, period. That some code is (supposedly) already correctly enforcing the invariant is immaterial, isn't it?
lib/grpnice.gi
Outdated
| @@ -103,6 +114,9 @@ InstallMethod( GroupByNiceMonomorphism, | |||
| function( nice, grp ) | |||
| local fam, pre; | |||
|
|
|||
| if not IsInjective( nice ) then | |||
| Error( "<nice> is not injective" ); | |||
There was a problem hiding this comment.
| Error( "<nice> is not injective" ); | |
| Error( "<nice> is not known to be injective" ); |
There was a problem hiding this comment.
Well, the current code actually checks injectivity, thus the message is correct.
The question is whether we should instead check HasIsInjective, and signal an error if not.
fingolfin
changed the title
SetNiceMonomorphism( G, map ) requires HasIsInjective( map ) and IsInjective( map )IsInjective set (otherwise an infinite recursion can happen)
There was a problem hiding this comment.
I approve in general but perhaps you may wish to address some or all of my comments.
BTW I don't really think this fits in the category "bug fix", but rather it improves the documentation and explicitly enforces an invariant, so it stops the user from shooting themselves into the foot. OK, so maybe it is a bug fix then, but a bug fix for the documentation / specification of nice monomorphisms. Not that it really matters, except for where to put this in the release notes 😂
|
Do we need / want this to be backported to 4.12 ? I have no issue with that, though strictly speaking it is not necessary, and any backported change has a risk of causing a regression somewhere... |
|
@fingolfin Thanks for your suggestions, I am going to update the pull request. Concerning the question whether the changes are a bugfix, I would argue that the starting point was an infinite recursion which is now avoided. Of course one gets an error in this situation also after the changes, but we have replaced an "unexpected error" (which is one of our bug classes) by an "expected error" (not a bug); Concerning the question whether we should backport the changes to 4.12, I have no strong opinion. On the one hand, the Oscar code that uncovered the problem is safe now; on the other hand, the same problem may be hit in other situations, and then the new error handling makes it easy to solve this problem. |
fingolfin
removed
the
topic: documentation
This addresses issue #5020.
We check whether the map that shall be set as nice monomorphism has already the
IsInjectiveflag, and signal an error if not.The documentation mentions/recommends in the introduction of the section on nice monomorphisms that one can set a known monomorphism with stored
IsInjectiveflag as a nice monomorphism by hand.(The Tutorial had already an example for that.)