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

Stop wrapping Julia modules in special objects #1049

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Sep 23, 2024

This resolves issues with outdated caches in our module wrappers.

We added these wrappers to serve as a cache. But we already exempted Julia functions from this. For which we create wrapper objects -- so arguably the cache would make most sense for them... But this can cause inconsistencies, hence @ThomasBreuer remove this in PR #423 four years ago.

So the only thing we cached were the wrapper objects for Julia modules. And the only reasons we needed those wrappers were ...

  1. to be able to do caching (haha),
  2. to be able to supply the "fake" Julia.GAP as a way to access the GAP Julia module,
  3. for some printing tweaks.

This PR achieves all of the above by adjusting the GAP type for Julia object slightly, we now indicate whether we are seeing a Julia module by setting a suitable filter. As an added bonus, this now also works with modules access by any other means, e.g. returned by a JuliaEvalString or a Julia function.

The part I like least is the new hack to make Julia.GAP possible. A long-term alternative to this might be to provide a different clean way to access this -- e.g. perhaps we should provide a global GAP variable GAP_jl which contains the GAP module? (We could also just call it GAP but I feel this might be reeeaaally confusing to GAP users, so I'd rather not). For symmetry in Oscar we could also add Oscar_jl as an alias for the Oscar global.

But all of that is beyond this PR. We could file it as a separate issue if care enough.

Resolves #1044
Closes #1045

@fingolfin
Copy link
Member Author

(This should only be merged after #1029, as getting that one right is much harder)

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.58%. Comparing base (e60382d) to head (5a8d7e0).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
pkg/JuliaInterface/gap/JuliaInterface.gi 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
- Coverage   75.61%   75.58%   -0.03%     
==========================================
  Files          55       55              
  Lines        4478     4477       -1     
==========================================
- Hits         3386     3384       -2     
- Misses       1092     1093       +1     
Files with missing lines Coverage Δ
pkg/JuliaInterface/gap/JuliaInterface.gd 100.00% <100.00%> (ø)
pkg/JuliaInterface/read.g 100.00% <ø> (ø)
pkg/JuliaInterface/src/JuliaInterface.c 74.81% <100.00%> (+0.57%) ⬆️
pkg/JuliaInterface/gap/JuliaInterface.gi 91.86% <90.00%> (-1.40%) ⬇️

Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

I don't feel qualified to comment on the changed internals here. Anyway, could you add a test for #1044, e.g. the one from https://github.com/oscar-system/GAP.jl/pull/1045/files#diff-f673fbc29b03e12764445c880278a94061eb0b24269b0fd2621c27c604d50b1e?

@fingolfin
Copy link
Member Author

@lgoettgens done, I shamelessly stole it from your PR :-)

@lgoettgens
Copy link
Member

@lgoettgens done, I shamelessly stole it from your PR :-)

I stole it from your issue, so I think we're even now :)

This resolves issues with outdated caches in our module wrappers.
@fingolfin fingolfin merged commit 6d0bbcf into oscar-system:master Sep 24, 2024
21 checks passed
@fingolfin fingolfin deleted the mh/unwrap-modules branch September 24, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing a Julia module is not handled well
2 participants