Open fingolfin opened 1 year ago
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:
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/.)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 )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 https://github.com/oscar-system/Oscar.jl/issues/1826#issuecomment-1424524312, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV4ALHQSMKNUSWM3QOTWWUPWTANCNFSM6AAAAAAS7CMP6U . You are receiving this because you are subscribed to this thread.Message ID: @.***>
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.
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.
The following might also be related/could be closed once the big renaming is complete: https://github.com/oscar-system/Oscar.jl/issues/1714
Some more names that I noted:
QQ["x","y","z"]
works but QQ[:x,:y,:z]
not. edit: Works now.
rational_field
integer_ring
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)
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 ?
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 ?
Returning a "generator" of ZZ
or QQ
seems like overkill to me.
I think just returning ZZ
respectively QQ
is fine.
What about more or less weirdly types in Hecke that currently do not fit into our naming scheme, e.g. NfRelNS, GrpAbFinGen or AlgMat?
I plan to rename matrix_algebra/MatrixAlgebra
from AbstractAlgebra to matrix_ring
, so that matrix_algebra
can be a matrix algebra.
The hackmd in https://github.com/oscar-system/Oscar.jl/issues/1826#issue-1497550384 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?
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
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:
[x] more general renaming:
[ ] short names that need longer alias (or be renamed outright?)
number_of_connected_components
?[ ] what about
sylow_subgroup
)