Add a RedispatchOnCondition to SubdirectProductOp

[stevelinton]

Description

SubdirectProductOp methods generally require IsGroupHomomorphsm of their third and fourth
arguments, but this includes filters which are Properties rather than Categories and may not be known.

This PR Adds a RedisptachOnCondition to deal with this, and also adds some family predicates that should make code more robust, and a test.

Text for release notes

Fix a bug with in calculating SubdirectProducts which could sometimes fail on valid input.

Further details

Bug was reported by Harry Braden.

Checklist for pull request reviewers

• proper formatting

If your code contains kernel C code, run clang-format on it; the
simplest way is to use git clang-format, e.g. like this (don't
forget to commit the resulting changes):

git clang-format $(git merge-base master)

• usage of relevant labels

  1. either release notes: not needed or release notes: to be added
  2. at least one of the labels bug or enhancement or new feature
  3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
  4. consider adding any of the labels build system, documentation, kernel, library, tests

• runnable tests

• adequate pull request title

• well formulated text for release notes

• relevant documentation updates

• sensible comments in the code

[coveralls]

Coverage increased (+0.002%) to 84.298% when pulling 8d3cb47 on stevelinton:subdirect into 1568bb1 on gap-system:master.

[fingolfin]

Is this supposed to fix #3431? If so, please add fixes #3431 to the PR description, so that merging this PR will close the issue.

[fingolfin]

All in all, I am sceptical. Isn't the true problem that the mappings returned by IsomorphismPcGroup do not have IsInjective set? Wouldn't fixing that be much simpler resp. less invasive, and at the same time, possible benefit other things as well?

Wonder if @hulpke has some thoughts on this PR?

[fingolfin]

Hmm, doesn't this mean the user will get strictly worse error messages now in case ghom and/or hhom really isn't a homomorphism? I.e. there will be a "no method found" error instead of this very specific information.

[fingolfin]

I don't actually understand why it is necessary and/or beneficial to remove this check here for the purpose of this PR?

[fingolfin]

Hm. So invalid arguments for SubdirectProductOp which this new family predicate will lead to "method not found" errors, instead of running into different (possibly more helpful) errors later on in the code (e.g. when the homomorphisms are applied and "notice" that they cannot actually be applied).

I guess one way out would be to add another method w/o family predicate, which checks if the family predicate is violated, and if so, prints a specific error message, and otherwise redispatches?

[stevelinton]

@fingolfin There is one point of principle here, which is that you shouldn't give "No Method Found" for an object which is entirely suitable for a Method except that it doesn't know some Properties which are in fact true. So one way or another, we should catch the case of mappings which are homomorphisms but don't know it. The independent fact that IsomorphismPcGroup sometimes produces such objects is just one way that this could happen.

So adding the RedispatchOnCondition call (or something which does the same at least in the case where the mappings are homomorphisms) seems to be necessary.

Fixing IsomorphismPcGroup also seems like an obvious move with no downside.

The remaining questions are to do with giving the most helpful possible error messages for actually invalid arguments. Here I have no strong feelings. The operation is expensive enough that the checks won't make any difference, so we could have a method with minimal requirements whose job is simply to give intelligible error messages for bad arguments and then TryNextMethod()

[hulpke]

I don't have a problem with this PR -- it resolves an issue of a property not being set and adds a family predicate that (lazily) was ignored 20 years ago.

However, independent of this, the IsomorphismPcGroup method for fp groups (respectively: EpimorphismSolvableQuotient if the size is known) is missing a "SetIsInjective`. I will add that (which also will resolve the problem).

[stevelinton]

@hulpke The second commit in this PR fixes IsomorphismPcGroup but doesn't handle the case of EpimorphismSolvableQuotient and known size. If you want to fix that more directly, I'll remove that change from this PR.

[hulpke]

@stevelinton
Sorry -- did not see. No, keep it in. I will set it separately in the EpimorphismSolvableGroup method, but there could be other methods in the future.

[fingolfin]

Something is wrong with this PR, there are some failed CI statuses, and some successful ones... @stevelinton perhaps you could rebase and force push this PR to trigger a fresh rebuild, hopefully resolving this?

[codecov]

Codecov Report

Merging #3437 into master will increase coverage by 7.3%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3437      +/-   ##
==========================================
+ Coverage   77.24%   84.54%    +7.3%     
==========================================
  Files         692      698       +6     
  Lines      339572   345290    +5718     
==========================================
+ Hits       262306   291942   +29636     
+ Misses      77266    53348   -23918

Impacted Files	Coverage Δ
lib/gprd.gi	74.58% <100%> (+23.45%)	⬆️
src/libgap-api.c	65.64% <0%> (-5.15%)	⬇️
src/gasman.c	86.28% <0%> (-5.04%)	⬇️
src/iostream.c	65.9% <0%> (-2.87%)	⬇️
src/hookintrprtr.c	67.34% <0%> (-2.55%)	⬇️
src/io.c	78.7% <0%> (-2.18%)	⬇️
src/vector.c	97.92% <0%> (-2.08%)	⬇️
src/vecffe.c	82.72% <0%> (-1.64%)	⬇️
src/sysfiles.c	41.39% <0%> (-1.59%)	⬇️
src/lists.c	75.83% <0%> (-1.4%)	⬇️
... and 283 more

[stevelinton]

Tests now seem to be fine. @fingolfin we could do more in terms of catching bad inputs and trying to give a more targeted error message, but there are lots of places where we could do that. SubdirectProduct (not the Op) does a bit of that anyway. Are you OK with merging this as is to get the actual bug fixed?

[fingolfin]

@stevelinton I agree that many other places could / should have better error messages, and that alone wouldn't be a concern for me. What bothers me is that this PR makes error messages worse in certain situations, and IMHO for no good reason.

To me, SubdirectProductOp clearly is a low level operation, not documented on purpose; that means to me that we can put some burden on callers to use it correctly. But for some reason, you removed the check before one of the two places that call SubdirectProductOp. Why is that necessary, resp. beneficial?

Wouldn't the minimally invasive solution here be to tweak the second call to SubdirectProductOp in the library, by inserting homomorphism checks before it, too?

Thing is, of course I want to get this bug fixed ASAP. And I also am happy and open to discuss tweaking the setup of things. I just don't like mixing the two needlessly.

[fingolfin]

Also, I would like to point out that I have not "requested changes" or otherwise "vetoed" this PR. But I also don't feel like approving it. As explained by me in the past, that means that if others feel it should be merged, so be it, I'll live with it. I just don't want to be the one who makes that decision.

[stevelinton]

@fingolfin this is a useful discussion. It's worth a modest amount of time to get the best code in place. I'm in no sense annoyed. Just wanted your input on what is the best thing to do.

The test in gprd.gi seemed to be subsumed by the RedispatchOnCondition which basically does the same thing. That's the only reason I removed it, however I agree it does potentially provide a better error message, so I'd have no problem putting it back (and in the other place) except that if you have two pieces of code to trap the same error, it is hard to test both of them.

If we believe that SubdirectProductOp will only ever be called from two places then indeed, we could put the checks in and document in comments that they are needed, however we have RedispatchOnCondition because this sort of thing happens quite a lot. I kind of feel that if we stop using our general tools because they aren't quite perfect we are going in the wrong direction. Better to add an issue to see if we can improve RedispatchOnCondition and/or the no method found handlers to be generally better (or at least allow things to be better by passing additional arguments).

[fingolfin]

First off, I am glad to hear you are not annoyed, because I truly and honestly do not want to annoy you, and of course I want the bug fix (and after all, we can change any change this PR makes now at a later point again).

Next point of order: I just checked, no deposited package ever calls SubdirectProduct or SubdirectProductOp; the only thing used is SubdirectProducts (which computes all conjugacy classes of subdirect products of two given groups).

Now, it only makes sense to call SubdirectProductOp if the provided generalized maps are single valued (at least I don't know how to define the subdirect product otherwise). So this is something required of its arguments. So we could indeed use RedispatchOnCondition here to "check" for that (as this PR does). But the problem with that is that this mechanism is not powerful enough to express the full story: it will not reject inputs without that filter. This means that (to me, at least) this situation fundamentally different from other cases where we use RedispatchOnCondition to express that knowing certain filters might enable additional methods, and potentially more efficient ones; but still the check is "optional": even if the check is not satisfied, a "method not found" error is (usually) appropriate, because in principle, there could be a method to do that, we just did not (yet?) provide one. Not so here: We can't provide a method that does something mathematically for generalized maps that are not single valued. Though we could provide one which catches that case and triggers an error.

So one way to view it is that perhaps there should be a generalized way to handle this; a way to specify "the Nth argument of this operation must have property X; if property X is not known, compute it first, before dispatching to any other method; also, if X is false abort with an error".

One way to do this might perhaps be to write a function one can call to signal this, and that function would then do something similar to RedispatchOnCondition, but slightly different: The former installs a method that checks whether at least one new property value was computed, and then redispatches; or else it does a TryNextMethod(). The new helper would check whether at least one new property value was computed and all specified properties are true and then redispatches; or else it calls Error.

But there is another approach to this problem in general, which we use already in plenty places of the library: it is to split the operation in two: one front end function which performs all checks, and the dispatches to the actual function. Which is precisely what SubdirectProduct is already doing: SubdirectProduct is a function which performs some validation, and then SubdirectProductOp relies on that validation having taken place. We only reason we call SubdirectProductOp in a second place is (I assume) to bypass that validation, i.e., efficiency; so SubdirectProductOp is kind of treated as if it was SubdirectProductNC.

Now, the advantage of the first approach is that it avoids the problem where somebody calls the "internal" variant w/o really having ensured all prerequisites are satisfied (though I'd like to point out that in the case at hand, this should not have been necessary anyway, a function which is supposed to return an isomorphism can be expected to return an isomorphism, after all).

A major drawback of the first approach is that it cannot actually perform all required checks! Namely, verifying that the images of the homomorphisms are equal cannot be done this way.

Thus, the second approach has the advantage that all checks are bundled in one place, and it is arguably easier to understand what's going on (less "magic").

Given that we already are using the second approach here, and that using RedispatchOnCondition or a variant thereof won't be able to take care of all the required input validation either, I am not sure why to bother? Even using the dedicated filter seems overkill, when we could perform the same check in SubdirectProduct.

Finally, completely independent of everything else, I find it odd that SubdirectProduct accepts homomorphisms which have images that are isomorphic but not equal, and that it then picks a random isomorphism to do its job. But the result then is not well-defined, as it depends on the chosen isomorphism. I don't understand why we do this, instead of simply raising an error? The documentation for SubdirectProduct does not mention this and only defines what happens if the homomorphisms have the same image. As @hulpke wrote that code, perhaps he can shed some light on this?

[wilfwilson]

@stevelinton: @fingolfin's #3485 has been merged, which contains a subset of this PR and fixes #3431. If you want your SubdirectProductFamilyPredicate and RedispatchOnCondition changes from this PR to be merged, then unfortunately you'll need to fix a conflict (it's not a major one I don't think). And then I'd recommend updating the PR title. Otherwise you could close the PR.

[fingolfin]

I took the liberty of rebasing this, and fixing typos in the commit message of the remaining commit (that message, however, still needs to be edited further).

[fingolfin]

@stevelinton could you please let us know if you have any intention on getting this merged in? Else we can just close it.

[DominikBernhardt]

@stevelinton Poke. What should happen with this PR?

[stevelinton]

@DominikBernhardt @fingolfin Missed the first poke, sorry. Will try and get to it before the end of this week

[stevelinton]

I've reduced this PR to a very small one that just adds the RedispatchOnCondition which I still think is correct.

[wilfwilson]

I've read through this thread, and understood the conversation. You make good points @fingolfin, there's certainly many ways in which the whole system can be improved. But for this now-small PR, I'm happy to approve it and get it merged.

(And as you've mentioned before @fingolfin, if it turns out to be a bad idea, we can always make changes in the future).