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

Introduce global gap variable Oscar_jl #4577

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

lgoettgens
Copy link
Member

Motivated by

If we went with GAP_jl, then for symmetry in Oscar we could also provide Oscar_jl as an alias for the Oscar global variable.

from oscar-system/GAP.jl#1053.

Copy link
Member

@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.

Thanks, just one comment should be changed.

############################################################################

# Use GAP operations for the serialization of GAP objects.
# (The methods will be Julia functions.)
Copy link
Member

Choose a reason for hiding this comment

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

The comment lines do not fit.
If we want to make a comment then perhaps "for backwards compatibility"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes, I just copied the block from above but forgot to update it. Will do that tomorrow

@@ -123,14 +123,14 @@ InstallMethod(SetNiceMorphismForJuliaMatrixRepGroup, [IsGroup], function(G)
Assert(0, ForAll(gens, IsJuliaMatrixRep));

ele := gens[1];
hom := Julia.Oscar.iso_oscar_gap(Julia.base_ring(ele!.m));
hom := Oscar_jl.iso_oscar_gap(Julia.base_ring(ele!.m));
Copy link
Member

Choose a reason for hiding this comment

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

We get warnings about Oscar_jl being unbound here. I think this might be caused by this code being loaded before Oscar.jl ?

The easiest way to fix this would be to move this file into OscarInterface.

Copy link
Member

Choose a reason for hiding this comment

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

(and then edit experimental/MatrixGroups/src/MatrixGroups.jl to remove its __init__ method)

Copy link
Member Author

Choose a reason for hiding this comment

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

Like this: 7f58958?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a general rule:
Any additional GAP code inside Oscar that shall become available in Oscar should be read via OscarInterface.
(If this is the case then perhaps it should be mentioned in the manual section "Interface functionalities beyond GAP.jl".)

@fingolfin fingolfin merged commit 579a22d into oscar-system:master Feb 13, 2025
27 of 32 checks passed
@lgoettgens lgoettgens deleted the lg/gap-Oscar_jl branch February 13, 2025 07:16
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: GAP release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants