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

Replace many @aliases by @deprecate_binding #1557

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

fingolfin
Copy link
Member

Also drop two obsolete redundant uses of @alias. Also includes the fixes from PR #1556.

Companion to Nemocas/AbstractAlgebra.jl#1469

src/Aliases.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from 87b5b63 to 806b8fa Compare October 18, 2023 13:10
@thofma
Copy link
Member

thofma commented Oct 18, 2023

No objection to the change, but I am curious. Is this official API? Or could this be gone in some minor version?

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch 2 times, most recently from 9ff5906 to c623d2f Compare October 18, 2023 20:23
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8dff2d9) 84.17% compared to head (1df6741) 84.16%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1557      +/-   ##
==========================================
- Coverage   84.17%   84.16%   -0.01%     
==========================================
  Files          94       94              
  Lines       36658    36658              
==========================================
- Hits        30857    30854       -3     
- Misses       5801     5804       +3     

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

@fingolfin
Copy link
Member Author

It is not official API, but it has been around since at least Julia 1.0, and there is an open issue requesting that it be made official.

All in all I think it is very unlikely that this is removed in a patch version of Julia. It might be removed in 1.11 or 1.12 or whatever (though I again think it is unlikely), but I hope that by then we'll have moved these one step further (either by switching them to use plain @deprecate, or by removing them outright).

@fingolfin
Copy link
Member Author

Oh, and: this breaks the CI tests of our downstream packages as they test with depwarn=error. So I guess we should consider this a breaking change... In any case, we probably should first fix all the uses of deprecated API names in our various packages before we merge this...

@lgoettgens
Copy link
Collaborator

Deprecating stuff is not breaking!
But yeah, we should fix our packages beforehand

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch 2 times, most recently from 734dc89 to 6424be4 Compare November 1, 2023 20:51
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch 3 times, most recently from aee6e39 to 79555e0 Compare November 7, 2023 10:13
@fingolfin
Copy link
Member Author

(Old comment follows which I wrote a week or so ago but apparently forgot to submit sigh):

The failures are due to the use of CyclotomicField; together with PR #1576, this PR now removes this if deprecations are set to error.

Indeed we use CyclotomicField as a kind of "fake type" in Oscar (and I now recall that @fieker pointed this out to me previously). Specifically:

experimental/GModule/GModule.jl:57:julia> C = gmodule(CyclotomicField, C);
experimental/GModule/GModule.jl:283:  return gmodule(AnticNumberField, gmodule(CyclotomicField, M))
experimental/GModule/GModule.jl:290:    a = gmodule(CyclotomicField, m)
experimental/GModule/GModule.jl:338:function irreducible_modules(::typeof(CyclotomicField), G::Oscar.GAPGroup)
experimental/GModule/GModule.jl:340:  return [gmodule(CyclotomicField, m) for m in z]
experimental/GModule/GModule.jl:345:  z = irreducible_modules(CyclotomicField, G)
experimental/GModule/GModule.jl:354:function gmodule(::typeof(CyclotomicField), C::GModule)
experimental/GModule/GModule.jl:1261:  C1 = gmodule(CyclotomicField, C)
experimental/GModule/GModule.jl:1262:  D1 = gmodule(CyclotomicField, D)
experimental/GModule/GModule.jl:1408:function Oscar.gmodule(T::Union{Type{CyclotomicField}, Type{AnticNumberField}}, chi::Oscar.GAPGroupClassFunction)
experimental/GModule/test/runtests.jl:111:  S = gmodule(CyclotomicField, R)
src/Rings/AbelianClosure.jl:426:function minimize(::typeof(CyclotomicField), a::AbstractArray{nf_elem})
src/Rings/AbelianClosure.jl:454:function minimize(::typeof(CyclotomicField), a::MatElem{nf_elem})
src/Rings/AbelianClosure.jl:455:  return matrix(minimize(CyclotomicField, a.entries))
src/Rings/AbelianClosure.jl:458:function minimize(::typeof(CyclotomicField), a::nf_elem)
src/Rings/AbelianClosure.jl:459:  return minimize(CyclotomicField, [a])[1]
src/Rings/AbelianClosure.jl:462:conductor(a::nf_elem) = conductor(parent(minimize(CyclotomicField, a)))
src/Rings/AbelianClosure.jl:684:hash(a::QQAbElem, h::UInt) = hash(minimize(CyclotomicField, a.data), h)

Note that even in OSCAR 0.13.0, this is just a generic function:

julia> CyclotomicField
cyclotomic_field (generic function with 9 methods)

How do we proceed? We can of course just keep this alias. But I wonder if this may not lead to other kinds of confusion down the line (after all, CyclotomicField is used here in a way that suggests it is a type, but it really isn't).

We could of course change Oscar to use cyclotomic_field. Or perhaps one could use QQab instead? I.e. minimize(QQab, a.entries) and

@thofma
Copy link
Member

thofma commented Nov 14, 2023

It could be replaced for minimize (these should be internal anyway), but for irreducible_gmodules(...) it would be wrong to replace it by QQab and irreducible_modules(cyclotomic_field, G) looks a bit odd.

@lgoettgens
Copy link
Collaborator

But I don't think that the name CyclotomicField needs to be provided by Nemo for some Oscar functionality. If Nemo does not use it, it shouldn't be there nor exported.

@fingolfin
Copy link
Member Author

Discussed it with @fieker: we'll remove CyclotomicField here and re-introduce it in Oscar, as suggested by @lgoettgens .

This way any access to them trigger a depwarning (if those are enabled,
as they are in our CI).

This is a first step to eventually removing these aliases.
They are already provided by AbstractAlgebra.
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from 79555e0 to 1df6741 Compare November 23, 2023 14:57
@fingolfin fingolfin merged commit bf3064e into Nemocas:master Nov 23, 2023
14 of 15 checks passed
@fingolfin fingolfin deleted the mh/deprecate_binding branch November 23, 2023 15:12
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.

3 participants