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

Fix QuaternionAlgebra bug that caused QuaternionAlgebra( [1], 2, 5 ).1 to error the second time it was invoked #4427

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 21, 2021

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 #2940

@fingolfin fingolfin added topic: HPC-GAP unification topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 21, 2021
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a 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" );
Copy link
Contributor

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.

Copy link
Member Author

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
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 21, 2021
@fingolfin fingolfin changed the title Change QuaternionAlgebra to use GET_FROM_SORTED_CACHE Fix QuaternionAlgebra bug that caused QuaternionAlgebra( [1], 2, 5 ).1 to error the second time it was invoked Apr 21, 2021
@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Apr 21, 2021
@ThomasBreuer ThomasBreuer self-requested a review April 23, 2021 08:25
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.

@ThomasBreuer ThomasBreuer merged commit 039de32 into gap-system:master Apr 23, 2021
@fingolfin fingolfin deleted the mh/QuaternionAlgebraData branch April 23, 2021 08:36
@wilfwilson wilfwilson removed the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: HPC-GAP unification topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HPC-GAP specific code for QuaternionAlgebraData
3 participants