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

Remove global Flint* objects #1935

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Conversation

lgoettgens
Copy link
Collaborator

As suggested by @fieker in oscar-system/Oscar.jl#1379 (comment).

I would consider this a breaking change for Nemo, and this possibly needs some changes downstream in Hecke and Oscar for uses of FlintZZ and FlintQQ.

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.96%. Comparing base (7833f8d) to head (89fe103).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1935      +/-   ##
==========================================
- Coverage   87.98%   87.96%   -0.02%     
==========================================
  Files          99       99              
  Lines       36458    36458              
==========================================
- Hits        32076    32070       -6     
- Misses       4382     4388       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lgoettgens
Copy link
Collaborator Author

Triage decided (ref oscar-system/Oscar.jl#1379 (comment)) to first replace all uses of these downstream, then wait for some time, and only then merge this. If we need deprecations or not may be discussed then.

@thofma
Copy link
Member

thofma commented Nov 6, 2024

can we keep the definitions but not export them? they are kind of handy

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Nov 6, 2024

can we keep the definitions but not export them? they are kind of handy

the gaussian ones or the QQ/ZZ? And what is the benefit of e.g. FlintQQ over QQ?
(just trying to understand your use-case)

@thofma
Copy link
Member

thofma commented Nov 6, 2024

The Gaussian ones

@lgoettgens
Copy link
Collaborator Author

We could just add QQi (instead of FlintQQi) as an unexported global for this (and similarly for ZZi)

@thofma
Copy link
Member

thofma commented Nov 6, 2024

also works for me

@fingolfin
Copy link
Member

As a milder step I would suggest that as a milder change we instead (or at first) modify Oscar and/or Hecke to not import resp. not re-export these. That would then perhaps be "breaking" for Oscar and/or Hecke but not Nemo, which arguably affects fewer people... But I am open to this PR here as well shrug

@lgoettgens
Copy link
Collaborator Author

@thofma just approved via slack

@lgoettgens lgoettgens merged commit a67724b into Nemocas:master Dec 13, 2024
23 of 24 checks passed
@lgoettgens lgoettgens deleted the lg/global-Flint-obj branch December 13, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants