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.

Sign up for GitHub

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

Jump to bottom

Regression: recent changes caused package guava to fail its test suite #5334

Closed
fingolfin opened this issue Jan 19, 2023 · 1 comment · Fixed by #5335
Closed

Regression: recent changes caused package guava to fail its test suite #5334

fingolfin opened this issue Jan 19, 2023 · 1 comment · Fixed by #5335
Labels
regression A bug that only occurs in the branch, not in a release

Comments

@fingolfin
Copy link
Member

First failing report is from 2023-01-12 at 15:27:16:

########> Diff in /home/runner/gap/pkg/guava-3.18/tst/guava.tst:355
# Input is:
WholeSpaceCode( 5, GF(4) ) = WholeSpaceCode( 5, GF(2) );
# Expected output:
false
# But found:
true
########

I have bisected the issue to commit 2f4a518 by @ThomasBreuer via PR #3006
@fingolfin fingolfin added the regression A bug that only occurs in the branch, not in a release label Jan 19, 2023
@ThomasBreuer
Copy link
Contributor

The real problem is a bug in COEFFS_SEMI_ECH_BASIS:

gap> V:= GF(2)^1;; B:= Basis( V, [ [ Z(2) ] ] );;
gap> Coefficients( B, [ Z(4) ] );
[ Z(2^2) ]

The correct result would be fail.

This problem shows up now because the changes from #3006 cause that a method for \in gets a lower rank, and thus another method (which uses COEFFS_SEMI_ECH_BASIS) is called in the computation in question. I will create a pull request in order to fix the bug.

ThomasBreuer added a commit to ThomasBreuer/gap that referenced this issue Jan 19, 2023
@ThomasBreuer
fix a bug in Coefficients
c922d27 
resolves gap-system#5334
@ThomasBreuer ThomasBreuer mentioned this issue Jan 19, 2023
Merged
ThomasBreuer added a commit to ThomasBreuer/gap that referenced this issue Jan 19, 2023
@ThomasBreuer
deal with unwanted side-effects of gap-system#3006
14d2c31 
- 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.)
@ThomasBreuer ThomasBreuer mentioned this issue Jan 19, 2023
Merged
fingolfin pushed a commit that referenced this issue Jan 19, 2023
@ThomasBreuer @fingolfin
fix a bug in Coefficients
1f90b51 
resolves #5334
fingolfin pushed a commit that referenced this issue Jan 19, 2023
@ThomasBreuer @fingolfin
deal with unwanted side-effects of #3006
38f2adb 
- 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.)
@ThomasBreuer ThomasBreuer mentioned this issue Jan 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
regression A bug that only occurs in the branch, not in a release
Projects
None yet
Milestone
No milestone
2 participants
@fingolfin @ThomasBreuer