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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.49%. Comparing base (0271fc0) to head (312639d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1935      +/-   ##
==========================================
- Coverage   87.51%   87.49%   -0.02%     
==========================================
  Files          97       97              
  Lines       35532    35538       +6     
==========================================
- Hits        31096    31095       -1     
- Misses       4436     4443       +7     

☔ 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

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