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

The big renaming #1376

Merged
merged 18 commits into from
Feb 23, 2023
Merged

The big renaming #1376

merged 18 commits into from
Feb 23, 2023

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 21, 2023

Companion to Nemocas/AbstractAlgebra.jl#1267, thofma/Hecke.jl#977, oscar-system/Oscar.jl#1972

This is WIP on the big type renaming, see also https://hackmd.io/0hrBHIUrRZG23xiZZ0NCEA?both for our notes (these are partially outdated and misleading, we are working on that).

One issue with this PR is that we rename

  • FqDefaultPolyRing => FqPolyRing
  • FqPolyRing => FqPolyRepPolyRing

So the name FqPolyRing is both an old and a new type name, albeit with different meanings, which is unfortunate.

Actually, this could be avoided because FqPolyRing is not the final name we want anyway... Right now we do these renamings:

  • FqDefaultFiniteField => FqField
  • FqDefaultMatSpace => FqMatrixSpace
  • FqDefaultMPolyRing => FqMPolyRing
  • FqDefaultPolyRing => FqPolyRing
    but we really would prefer to just call this FinField, FinFieldMatrixSpace, etc. (or perhaps FiniteField, ...?) but FinField is already an abstract type in AbstractAlgebra... We could rename that, or just re-use the name (by not importing FinField from AA into Nemo), but all of these involve further discussions and decisions which is why I did not yet attempt this here.

Also missing are the various SeriesRings/SeriesElem. I'll work on those tomorrow

And of course there are more things to be done in AA, Oscar, Hecke

TODOs (either in this PR or a follow up):

  • also rename _series types
  • consider updating filenames (such as fmpz.jl), OR if we keep them, ensure we reference them correctly in code and docs
  • update documentation (e.g. docs/src/developer/introduction.md:58) with a table translating old "Flint names" like fmpz to new "Nemo names" like ZZRingElem
  • add docstrings to all types that
    - explain what the type represents
    - mentions Flint names, if applicable
    - gives an example how to create an instance (ideally as a jldoctest)
  • ...

@fingolfin
Copy link
Member Author

Also, we wanted to rename arb to RealFieldElem and more arb/acb renaming, but then discovered that there is already RealElem which seems to be almost identical to arb except for the parents, and so maybe we need some further discussion, perhaps with @thofma ?

@thofma
Copy link
Member

thofma commented Feb 21, 2023

I merged #1367 yesterday, because it is related.

To quote from #1367 (comment)

I don't want to hear any discussions about the names. If they need to be adjusted, it can be done during the "big rename"

Ideally a user will never see an arb/acb again, so I would leave them as they are now. Rename the RealElem etc. as much as you want. I am also happy to discuss this.

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #1376 (06952bc) into master (c152bcb) will increase coverage by 0.02%.
The diff coverage is 78.77%.

@@            Coverage Diff             @@
##           master    #1376      +/-   ##
==========================================
+ Coverage   88.43%   88.45%   +0.02%     
==========================================
  Files          88       88              
  Lines       34327    34327              
==========================================
+ Hits        30356    30363       +7     
+ Misses       3971     3964       -7     
Impacted Files Coverage Δ
src/Nemo.jl 27.48% <ø> (ø)
src/ambiguities.jl 0.00% <0.00%> (ø)
src/calcium/CalciumTypes.jl 100.00% <ø> (ø)
src/flint/FlintTypes.jl 94.81% <ø> (ø)
src/flint/flint_puiseux_series.jl 81.49% <ø> (ø)
src/flint/fmpq_mpoly.jl 86.29% <ø> (ø)
src/flint/fmpq_poly.jl 94.73% <ø> (ø)
src/flint/fmpq_rel_series.jl 94.40% <ø> (ø)
src/flint/fmpz.jl 92.81% <ø> (ø)
src/flint/fmpz_abs_series.jl 94.84% <ø> (ø)
... and 79 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fingolfin

This comment was marked as resolved.

@thofma

This comment was marked as outdated.

@fingolfin
Copy link
Member Author

I merged #1367 yesterday, because it is related.

Thank you, I hadn't noticed, and will read up there now.

To quote from #1367 (comment)

I don't want to hear any discussions about the names. If they need to be adjusted, it can be done during the "big rename"

Oh sure -- I didn't mean to complain about the names there (or anything, for that matter), it was just a bit of confusion yesterday when we discovered RealElem vs. arb and where unsure how to continue.

But now this seems clearer:

Ideally a user will ever see an arb/acb again, so I would leave them as they are now. Rename the RealElem etc. as much as you want. I am also happy to discuss this.

So basically we use RealElem (possibly renaming it to RealFieldElem, etc., etc and ignore arb and acb and friends. And then in Oscar.jl, I think the only places referencing them are in the serialization code, so I guess that code should be changed to use RealElem etc. (And for Hecke, I am sure you already have a plan, so we'll not touch it there)

@thofma
Copy link
Member

thofma commented Feb 22, 2023

Yes, that sounds good.

@fingolfin
Copy link
Member Author

I've now renamed RealElem to RealFieldElem and ComplexElem to ComplexFieldElem.

Alas, there is already a type RealFieldElem in Hecke, for a completely different purpose, so I'll have to rename that first now ...

@thofma
Copy link
Member

thofma commented Feb 22, 2023

there is a reason it is big renaming and not just renaming :)

@fingolfin fingolfin marked this pull request as ready for review February 23, 2023 14:57
@thofma
Copy link
Member

thofma commented Feb 23, 2023

Is squash fine?

@thofma thofma enabled auto-merge (squash) February 23, 2023 15:14
@fingolfin
Copy link
Member Author

The branch protection rules are confused by the fact that we are not testing against the master branch of upstream, but rather mh/rename. So we can't use automerge sigh

@fingolfin fingolfin merged commit a6a50c4 into Nemocas:master Feb 23, 2023
@fingolfin
Copy link
Member Author

ARGH I accidentally rebased instead of a merge. I'll fix it

@fingolfin fingolfin deleted the mh/rename branch February 23, 2023 15:40
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