Skip to content
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

Regression in DeclareHandlingByNiceBasis/InstallHandlingByNiceBasis #5322

Closed
zickgraf opened this issue Jan 13, 2023 · 3 comments
Closed

Regression in DeclareHandlingByNiceBasis/InstallHandlingByNiceBasis #5322

zickgraf opened this issue Jan 13, 2023 · 3 comments
Labels
regression A bug that only occurs in the branch, not in a release

Comments

@zickgraf
Copy link
Contributor

The documentation of DeclareHandlingByNiceBasis/InstallHandlingByNiceBasis claims:

name must be a string, a filter f with this name is created, and a logical implication from f to IsHandledByNiceBasis (61.11-6) is installed.

This is not true anymore since commit 2f4a518 (see 2f4a518#diff-14aba9b3712179af58d64fc6572ca6107a7c746245c8c795a98b79ec746676d4R690-L694).

This leads to an error when loading QPA2:

Error, required filters [
    "IsSubspaceOfQPAVectorSpace" 
]
for 1st argument do not match a declaration of NiceFreeLeftModule called from
<<compiled GAP code>> from GAPROOT/lib/oper1.g:336 in function InstallMethod called from
<function "InstallMethod">( <arguments> )
 called from read-eval loop at QPA2/lib/vecspace.gi:766
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue

I'm not sure if the documentation or the regressing commit should be updated.

@fingolfin fingolfin added the regression A bug that only occurs in the branch, not in a release label Jan 13, 2023
@ThomasBreuer
Copy link
Contributor

@zickgraf Yes, the new filter f is now just a flag such that f and IsFreeLeftModule and IsAttributeStoringRep implies IsHandledByNiceBasis. I think this is an improvement over the situation before #3006 got merged, and I am going to update the documentation accordingly.

The mentioned problem in the QPA2 package arises from the fact that the NiceFreeLeftModule method wants to be applicable as soon as the argument is in IsSubspaceOfQPAVectorSpace, which does no longer imply IsFreeLeftModule.
(Most of the vector spaces types in the GAP library use the generic NiceFreeLeftModule method, that's why I had overlooked this problem.)
If it is acceptable to adjust the QPA2 code then I would suggest to change the InstallMethod call such that IsFreeLeftModule and IsSubspaceOfQPAVectorSpace is required.

@zickgraf
Copy link
Contributor Author

If it is acceptable to adjust the QPA2 code then I would suggest to change the InstallMethod call such that IsFreeLeftModule and IsSubspaceOfQPAVectorSpace is required.

I have created a PR: sunnyquiver/QPA2#44 I'm not the author of QPA2, so I'm not 100% sure that this will be accepted, let's see.

ThomasBreuer added a commit to ThomasBreuer/gap that referenced this issue Jan 16, 2023
Since pull request gap-system#3006 go merged, the statement about the
implication installed by `InstallHandlingByNiceBasis` was not
correct.

This change addresses issue gap-system#5322.
fingolfin pushed a commit that referenced this issue Jan 18, 2023
Since pull request #3006 go merged, the statement about the
implication installed by `InstallHandlingByNiceBasis` was not
correct.

This change addresses issue #5322.
ThomasBreuer added a commit to ThomasBreuer/gap that referenced this issue Jan 19, 2023
- Let the filters created by `DeclareHandlingByNiceBasis` imply
  `IsFreeLeftModule`.
  This was the case before the changes from gap-system#3006, and this change fixes
  the problem described in gap-system#5322, as I had sketched in the discussion of gap-system#5325.
- Document this change (following gap-system#5325).
- Increase the rank of `IsHandledByNiceBasis` by 2.
  Then we get back to the rank before the changes from gap-system#3006.
  This way, the `\in` method with second argument in
  `IsFreeLeftModule and IsHandledByNiceBasis` is again ranked higher than
  the one with second argument `IsFreeLeftModule and IsFiniteDimensional`.
  The bug described in issue gap-system#5334 which has been found because of the
  reordering of these two methods (due to gap-system#3006)
  gets fixed via pull request gap-system#5335,
  now we can return to the better method ordering.
  (I do not like uprankings, but I have no better idea to solve this problem.)
fingolfin pushed a commit that referenced this issue Jan 19, 2023
- Let the filters created by `DeclareHandlingByNiceBasis` imply
  `IsFreeLeftModule`.
  This was the case before the changes from #3006, and this change fixes
  the problem described in #5322, as I had sketched in the discussion of #5325.
- Document this change (following #5325).
- Increase the rank of `IsHandledByNiceBasis` by 2.
  Then we get back to the rank before the changes from #3006.
  This way, the `\in` method with second argument in
  `IsFreeLeftModule and IsHandledByNiceBasis` is again ranked higher than
  the one with second argument `IsFreeLeftModule and IsFiniteDimensional`.
  The bug described in issue #5334 which has been found because of the
  reordering of these two methods (due to #3006)
  gets fixed via pull request #5335,
  now we can return to the better method ordering.
  (I do not like uprankings, but I have no better idea to solve this problem.)
@zickgraf
Copy link
Contributor Author

Fixed by #5336.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression A bug that only occurs in the branch, not in a release
Projects
None yet
Development

No branches or pull requests

3 participants