-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix QuaternionAlgebra
bug that caused QuaternionAlgebra( [1], 2, 5 ).1
to error the second time it was invoked
#4427
Fix QuaternionAlgebra
bug that caused QuaternionAlgebra( [1], 2, 5 ).1
to error the second time it was invoked
#4427
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, the caching technicalities are delegated to GET_FROM_SORTED_CACHE
.
The test failures are due to a subtle change of the behaviour.
After the changes, each call of QuaternionAlgebra
creates a new algebra, and the \.
access to generators is not supported since the filter IsFullSCAlgebra
is not set.
The situation was much worse with the old code: The first call for each coefficients family simply took the newly created algebra, in which the filter was set, whereas subsequent calls used the cache and then created a new algebra, without the filter.
Thus it depended on the GAP session whether \.
access to generators worked.
I regard the changes as a bugfix, perhaps even an entry for the release notes would be appropriate.
fi; | ||
|
||
fi; | ||
A:= AlgebraWithOne( F, GeneratorsOfAlgebra( stored ), "basis" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here a new algebra gets created; setting the filter IsFullSCAlgebra
would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, fixed now, and commit message adjusted. I agree this is a bugfix.
Also ensure that the exact same code is used to produce the resulting algebra regardless of whether it is created freshly or from the cache. This revealed a bug: `A.1` only worked in the former case, not the latter, which could lead to subtly broken code. Before this patch, the following happened: gap> A:= QuaternionAlgebra( [1], 2, 5 ); <algebra-with-one of dimension 4 over Rationals> gap> A.1; e gap> B:= QuaternionAlgebra( [1], 2, 5 ); <algebra-with-one of dimension 4 over Rationals> gap> B.1; Error, illegal access to record component `obj.1' This is now fixed. Resolves gap-system#2940
a2e57bf
to
85b5584
Compare
QuaternionAlgebra
bug that caused QuaternionAlgebra( [1], 2, 5 ).1
to error the second time it was invoked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
Also ensure that the exact same code is used to produce the
resulting algebra regardless of whether it is created freshly
or from the cache. This revealed a bug:
A.1
only workedin the former case, not the latter, which could lead to subtly
broken code. Before this patch, the following happened:
This is now fixed.
Resolves #2940