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... #1826

Open
4 of 29 tasks
fingolfin opened this issue Dec 14, 2022 · 16 comments
Open
4 of 29 tasks

The big renaming... #1826

fingolfin opened this issue Dec 14, 2022 · 16 comments
Labels
Milestone

Comments

@fingolfin
Copy link
Member

fingolfin commented Dec 14, 2022

It seems we never made an issue for this (or at least I can't find it...) and since @HereAround asked me about it to day:

There are ton of functions and types we think we should rename. We meant to do that for a long time now... Part of the problem is that in not all cases do we know what we want to rename to, i.e.: decisions need to be made. But that we can (and should!) do. Then "just" someone has to take care of it.

Note: I don't think we should strive to do "everything at once". Instead, we can start cleaning up things immediately whenever we can. But for some things, clearly we should first have the "big picture" that we aim for settled.

UPDATE: while we had a workshop where we did a ton of renaming, this is still not complete. Here is a list of more suggestions which we might want to consider:

  • more general renaming:

    • GradedPolynomialRing -> graded_polynomial_ring (?)
    • FiniteField -> finite_field
  • short names that need longer alias (or be renamed outright?)

    • gen -> generator
    • gens -> generators
    • conj -> conjugate
    • conj! -> conjugate!
    • minpoly -> minimal_polynomial?
    • charpoly -> characteristic_polynomial?
    • n_connected_components -> number_of_connected_components ?
    • n_maximal_cells
    • n_maximal_cones
    • n_maximal_polyhedra
    • n_nodes
    • haspreimage
      • urgh there is even a warning saying this:

        Do not confuse haspreimage with the function has_preimage, which
        works on variable of type GrpGenToGrpGenMor

  • what about

    • nrays -> number_of_rays?
    • nrels
    • ngens
    • ncols
    • nv
    • ne
    • Hecke.t2
    • Hecke.norm2
    • psylow_subgroup (conflict with sylow_subgroup)
    • quotient (exists!) vs. quo
    • intersection vs. intersect
@HereAround
Copy link
Member

I understand that this is to be addressed at the retreat in a few weeks. Here are two points in the toric universe, that should be considered:

  • Provide snake_case for all functions but types. (This is how I understand the first bullet point in the OSCAR style guide: https://docs.oscar-system.org/stable/DeveloperDocumentation/styleguide/.)
  • I wonder about functions whose name contains torusinvariant or torusfactor. Our standard reference (the book by Cox, Little, Schenk) spells these as "torus-invariant" and "torus factor" respectively. Does torusinvariant -> torus-invariant and has_torusfactor -> has_torus_factor, respectively, makes sense? (@lkastner )

@fieker
Copy link
Contributor

fieker commented Feb 9, 2023 via email

@HereAround
Copy link
Member

Torus-factor is illegal I think... The other one is e.g British Vs American spelling. Supporting both is fine, but tab completion cannot cope...

On Thu, 9 Feb 2023, 18:05 Martin Bies, @.> wrote: I understand that this is to be addressed at the retreat in a few weeks. Here are two points in the toric universe, that should be considered: - Provide snake_case for all functions but types. (This is how I understand the first bullet point in the OSCAR style guide: https://docs.oscar-system.org/stable/DeveloperDocumentation/styleguide/ .) - I wonder about functions whose name contains torusinvariant or torusfactor. Our standard reference (the book by Cox, Little, Schenk) spells these as "torus-invariant" and "torus factor" respectively. Does torusinvariant -> torus-invariant and has_torusfactor -> has_torus_factor, respectively, makes sense? @. https://github.com/lkastner ) — Reply to this email directly, view it on GitHub <#1826 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV4ALHQSMKNUSWM3QOTWWUPWTANCNFSM6AAAAAAS7CMP6U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Ok. This sounds like we should leave as is. Was just curious to hear what people think.

@HereAround
Copy link
Member

HereAround commented Feb 17, 2023

Here is another point - originally raised by @afkafkafk13:

|julia> C = positive_hull([1 0; 0 1])
A polyhedral cone in ambient dimension 2

|julia> antv = AffineNormalToricVariety(C)
A normal, affine toric variety

|julia> affine_toric_scheme = ToricSpec(antv)
Spec of an affine toric variety with cone spanned by RayVector{fmpq}[[1, 0], [0, 1]]

The suggestion by @afkafkafk13 was to remove the "A" in the output printed by the first two commands, i.e. the example should eventually look like:

|julia> C = positive_hull([1 0; 0 1])
Polyhedral cone in ambient dimension 2

|julia> antv = AffineNormalToricVariety(C)
Normal, affine toric variety

|julia> affine_toric_scheme = ToricSpec(antv)
Spec of an affine toric variety with cone spanned by RayVector{fmpq}[[1, 0], [0, 1]]

I briefly discussed with @fieker, who confirms that removal of the "A" is desired. The necessary change would affect the toric geometry and polyhedral geometry and seems well suited to be integrated into the big renaming. Hence, I post this here.

I have not checked all of OSCAR thoroughly to see what areas use which "A" convention. However, a first superficial check showed that rings, groups, schemes do not print the "A". So those areas will most likely not require a change.

@HereAround
Copy link
Member

HereAround commented Feb 21, 2023

The following might also be related/could be closed once the big renaming is complete: #1714

@simonbrandhorst
Copy link
Collaborator

simonbrandhorst commented Mar 20, 2023

Some more names that I noted:
QQ["x","y","z"] works but QQ[:x,:y,:z] not. edit: Works now.
rational_field
integer_ring

@fingolfin
Copy link
Member Author

QQ[:x,:y,:z] works now.

@simonbrandhorst I am not 100% what you mean with rational_field and integer_ring -- should these be aliases for QQ and ZZ or for QQField and ZZRing ? (i.e. would rational_field be a ring or would rational_field() be). (To be clear I think it's good to add these, just need to decide how)

@simonbrandhorst
Copy link
Collaborator

Since it is lower case, I would expect some kind of method.
So integer_ring() would be a ring I think. Is there any precedence of this kind ?

@fingolfin
Copy link
Member Author

That's a good argument then. Not sure what kind of precedent you are looking for? I guess finite_field(5) and polynomial_ring and cylotomic_field are all relevant examples... They all also return a pair consisting of the ring/field plus a generator... Perhaps integer_ring() and rational_field() should also do that? @fieker @thofma ?

@simonbrandhorst
Copy link
Collaborator

Returning a "generator" of ZZ or QQ seems like overkill to me.

@thofma
Copy link
Collaborator

thofma commented Sep 27, 2023

I think just returning ZZ respectively QQ is fine.

@fingolfin
Copy link
Member Author

Added in Nemocas/Nemo.jl#1547

@lgoettgens
Copy link
Member

What about more or less weirdly types in Hecke that currently do not fit into our naming scheme, e.g. NfRelNS, GrpAbFinGen or AlgMat?

@thofma
Copy link
Collaborator

thofma commented Dec 20, 2023

I plan to rename matrix_algebra/MatrixAlgebra from AbstractAlgebra to matrix_ring, so that matrix_algebra can be a matrix algebra.

@lgoettgens
Copy link
Member

The hackmd in #1826 (comment) mentiones a change like NCRing -> Ring -> CommRing. Is this still something to be addressed before 1.0 or do we conclude that this is too late / too much of a break?

@fingolfin
Copy link
Member Author

We decided back then to leave it at Ring and NCRing and not pursue this further. In any case, even if we still wanted to it, it would be a massive breaking change, also for a ton of external scope, it's way too late for that in 1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants