Ep/ Rename Spec to AffineScheme #3345 #3425

paemurru · 2024-02-23T21:06:09Z

Resolves #3345

Use `AffineScheme` only for the type, not the function.

As was recommended by Martin Bies and Wolfram Decker.

For biproducts, naming the output triple [obj], inj, pr seems to be common. Among the changes was changing "proj" to "pr" in the triples [obj], inj, proj

Also some other minor fixes

In particular, spec allows the argument to be a quotient ring or localized ring. Similarly for proj. Probably the tests would not have passed in the previous commit because of this

proj(Q::MPolyQuoRing{MPolyDecRingElem{T, PT}}) where {T, PT<:MPolyRingElem{T}} = ProjectiveScheme(Q)

joschmitt · 2024-02-23T21:46:54Z

experimental/QuadFormAndIsom/test/setup_tests.jl

@@ -1 +0,0 @@
-../../../test/Serialization/setup_tests.jl


This file is actually a symbolic to ../../../test/Serialization/setup_tests.jl. I assume it is a copy now because you used sed. (At least that's what happened to me in the past.)

Fixed. I don't know if there are other symbolic links ruined

codecov · 2024-02-23T21:51:53Z

Codecov Report

Merging #3425 (d52731f) into master (5d19430) will increase coverage by 0.00%.
The diff coverage is 80.36%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3425   +/-   ##
=======================================
  Coverage   81.89%   81.90%           
=======================================
  Files         561      561           
  Lines       75125    75128    +3     
=======================================
+ Hits        61523    61532    +9     
+ Misses      13602    13596    -6

Files	Coverage Δ
experimental/Experimental.jl	`81.81% <ø> (ø)`
...xperimental/LieAlgebras/src/LieAlgebraModuleHom.jl	`85.63% <ø> (ø)`
...dRealizationSpaces/src/MatroidRealizationSpaces.jl	`100.00% <100.00%> (ø)`
.../MatroidRealizationSpaces/src/realization_space.jl	`88.25% <ø> (ø)`
...rimental/MatroidRealizationSpaces/test/runtests.jl	`100.00% <ø> (ø)`
experimental/QuadFormAndIsom/src/embeddings.jl	`96.38% <100.00%> (ø)`
experimental/QuadFormAndIsom/src/enumeration.jl	`96.62% <100.00%> (ø)`
...ntal/QuadFormAndIsom/src/lattices_with_isometry.jl	`99.44% <ø> (ø)`
experimental/Schemes/AlgebraicCycles.jl	`76.37% <ø> (ø)`
experimental/Schemes/Auxiliary.jl	`94.38% <100.00%> (ø)`
... and 107 more

Remove commented question that I made earlier. Better to make an issue or ask someone than comment the code.

lgoettgens · 2024-02-23T22:49:42Z

experimental/LieAlgebras/src/LieAlgebraModuleHom.jl

@@ -403,7 +403,7 @@ function canonical_projection(V::LieAlgebraModule, i::Int)
  @req fl "Module must be a direct sum"
  @req 1 <= i <= length(Vs) "Index out of bound"
  j = sum(dim(Vs[l]) for l in 1:(i - 1); init=0)
-  proj = hom(
+  projection = hom(


What is the purpose of this change and the similar ones in parts of the code that have nothing to do with schemes? I don't really see why they are necessary.

#3345 (comment)

I am with @thofma here: #3345 (comment)

thofma

Variables can be called whatever the original author wants (unless of course it results now in a conflict). This should not be changed here.

lgoettgens · 2024-02-24T08:55:22Z

I think this can be achieved by partially reverting cb13aad

paemurru · 2024-02-24T10:58:30Z

@thofma @lgoettgens. The changes in cb13aad are mostly about how to name the maps returned by direct sums and products. In algebraic geometry, naming the returned projection map proj conflicts with the homogeneous spectrum. In areas that have connections to algebraic geometry, it is more subjective where to draw the line, where to keep proj for internal variable names and where not.

In general, I believe there is value in having consistent naming for variables returned in direct sums and products, so I mostly went with inj and pr, as that was already common.

OSCAR project leader Michael Joswig encouraged not using proj outside of the algebraic geometry meaning, see #3345 (comment). Unless you can quickly convince one of the OSCAR project leaders why your point of view is better, I don't think this justifies blocking the merging, especially given our time constraints. If you wish to later change some of the internal variable names back to proj in a different pull request, you are welcome to, it is a trivial matter.

paemurru · 2024-02-24T13:12:54Z

Do I understand correctly that you wish to revert the changes to variables named proj in the sense of a projection in local scope in cb13aad, regardless of whether they appear in group theory, algebra or algebraic geometry, regardless of whether they could be confused with the homogeneous spectrum?

fieker · 2024-02-24T15:33:16Z

There is no reason that I can see why an internal variable change is warranted.
If there is a convincing reason, I'd like learn it

paemurru · 2024-02-24T16:48:23Z

@fieker Easier to maintain code, find errors in code, collaborate with others. The same reason why style guides exist in most programming languages, why linters and automatic code formatters exist. The same reason why all code, including internal functions and variables in local scope, are usually formatted according to the style guide.

In this particular case, it is a common best practice not name a variable after functions or variables in global scope (see StackOverflow). It is something that linters warn against (I haven't found any currently maintained Julia linter, but here is an example from Pylint).

paemurru · 2024-02-24T16:59:02Z

Just to reiterate, I am happy to change the code as you suggest. I just want to make sure I understand what it is you wish to change, as it seems strange to me to use ´proj´ in algebraic geometry for projection morphisms where it can easily be confused with the homogeneous spectrum. It seems strange to me to go against standard best practices and to go against an OSCAR project leader's recommendation.

This renaming issue has been going on for weeks, with different people having different points of view, going back and forth on what or how to rename and with me having to accommodate to these new decisions. I am happy to make the changes, I just hope that I don't need to undo these changes again.

thofma · 2024-02-24T17:21:37Z

As I already wrote earlier, I think this is about everything in cb13aad (but the very few changes regarding to projectivization in it).

.

paemurru · 2024-02-25T17:54:28Z

Undid all the changes in cb13aad except for proj -> projectivization, as requested by @thofma. This includes changes in files not relating to schemes as well as changes in Schemes, changes in documentation and changes that had been approved by the original author in private communication.

thofma · 2024-02-25T20:43:31Z

Thanks! Maybe @simonbrandhorst could give a quick glance and approve the changes here?

fingolfin · 2024-02-26T08:18:11Z

@paemurru since you keep referring to "an OSCAR project leader's recommendation", let me point out that @fieker and @thofma also are OSCAR project leaders and as such I find it odd to draw that card against them... You can of course request that "the OSCAR leaders" sit together and give a final verdict before you change anything, so that you don't end up changing things back and forth (a quite reasonable request).

src/AlgebraicGeometry/Schemes/ProjectiveSchemes/Objects/Attributes.jl

simonbrandhorst · 2024-02-26T08:20:46Z

src/AlgebraicGeometry/Schemes/ProjectiveSchemes/Objects/Constructors.jl

+  is_standard_graded(S) || error("ring must be standard graded")
+  return ProjectiveScheme(S)


It seems to me that the better place for this input check would be the actual constructor
ProjectiveScheme? (however this need not delay merging this pull request.)

src/Groups/directproducts.jl

src/Rings/MPolyQuo.jl

src/deprecations.jl

simonbrandhorst · 2024-02-26T08:34:36Z

src/exports.jl

-# execute:  sort < src/exports.jl > dummy ; mv dummy src/exports.jl
+# execute:  LC_COLLATE=C sort < src/exports.jl > dummy ; mv dummy src/exports.jl


Good catch!

simonbrandhorst · 2024-02-26T08:41:48Z

src/exports.jl

-export projective_scheme
 export projective_space


Please keep the support for projective_scheme.
While proj is the mathematically correct name for this construction in algebraic geometry.
Oscar is for a wider audience. The name projective_scheme is more descriptive of what happens.
(construct a ProjectiveScheme from a graded ring).
Therefore both proj and projective_scheme should be allowed.
Similarly we should have the constructors affine_scheme and spec.

There are already some examples of how to achieve such an alias in Aliases.jl

Maybe someone else wants to use the abbreviations
proj or spec in another context with a different meaning.
Therefore I think that a global alias proj = projective_scheme is not justified here?
I merely added calls for projective_scheme with the corresponding signatures.

simonbrandhorst

Thanks for the big effort Erik. I appreciate it!

paemurru · 2024-02-26T10:18:32Z

@fingolfin I did not know that @thofma was among OSCAR project leaders, sorry about that. His name does not appear under OSCAR project leaders in the OSCAR website. Perhaps the page should be updated.

You can of course request that "the OSCAR leaders" sit together and give a final verdict before you change anything, so that you don't end up changing things back and forth (a quite reasonable request).

Yes, this is what I would have liked, as it seemed to me there was no consensus. But considering how everyone is very busy with the OSCAR book and OSCAR 1.0, I didn't think it would be appropriate for me to request this

* Rename Spec -> AffineScheme Use `AffineScheme` only for the type, not the function. * Rename proj for toric divisors to projectivization As was recommended by Martin Bies and Wolfram Decker. * Function ProjectiveScheme to projective_scheme and proj * Fix: spec now extends the constructor AffineScheme In particular, spec allows the argument to be a quotient ring or localized ring. Similarly for proj. Probably the tests would not have passed in the previous commit because of this * add support for projective_scheme and affine_scheme --------- Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de> (cherry picked from commit b00d5e4)

fingolfin · 2024-02-27T10:40:50Z

Yes, the website should be updated. We'll get to it eventually.

- Add QQBar docs to the manual #3423 - do not show the OscarInterface banner #3422 - fix bugs in all_OD_infos #3419 - Ep/ Rename Spec to AffineScheme #3345 #3425 - Remove two mentions of Arb_jll #3431 - Tweak epimorphism_from_free_group #3430 - CI: re-enable nightly #3435 - support gen(G::GAPGroup, 0) #3332 - Align all_*_groups methods some more #3433 - Add all_perfect_groups #3434 - Add all_primitive_groups and all_transitive_groups variants taking a single int or int range #3404 - fix a docstring #3436 - Fixes multivariate division #3396 - Docu invariants tori #3428 - Improve docstrings for is_conjugate/is_conjugate_with_data. #3384 - Fix ambient_module(M::SubquoModule) #3448 - Bugfix for printing of affine schemes #3437 - Bugfix for bugfix for printing of affine schemes #3445 - Update OSCAR banner #3410 - Docu invariants lin. red. groups (Lakshmi Ramesh and Wolfram Decker) #3443 - add od_from_atlas_group, od_from_p_subgroup, and helpers #3444 - Unexport normalise #3453 - support group properties for character tables #3449 - add docstrings for acting_group and action_function #3432 (exports are used in new groups code for the book) - Adjust to renaming of rank(A::FinGenAbGroup) to torsion_free_rank(A::FinGenAbGroup) #3457 - Ensure fp_group(G) transfers group attributes #3464 - Added comment on convention #3467 - Export weierstrass_chart_on_minimal_model and patch transform_to_weierstrass #3458 - Fix a doc signature #3466 - Grading + caching for affine algebra of torus invariants #3469

paemurru added 19 commits February 23, 2024 20:57

Rename Spec -> AffineScheme

741bd5d

Fix missed Spec -> AffineScheme

5cceb01

Fix: rename one more Spec to AffineScheme

aacac2e

Fix: change three more Spec's to AffineScheme

9b7ba2f

Rename the function AffineScheme to spec

7ea5a43

Use `AffineScheme` only for the type, not the function.

Rename proj for toric divisors to projectivization

400fc1e

As was recommended by Martin Bies and Wolfram Decker.

Change non-scheme proj functions to something else

cb13aad

For biproducts, naming the output triple [obj], inj, pr seems to be common. Among the changes was changing "proj" to "pr" in the triples [obj], inj, proj

Function ProjectiveScheme to projective_scheme

b4c5989

Also some other minor fixes

Fix: spec now extends the constructor AffineScheme

587ceeb

In particular, spec allows the argument to be a quotient ring or localized ring. Similarly for proj. Probably the tests would not have passed in the previous commit because of this

rename filename

b30d8c9

Fix tests for proj

32c2c30

projecitve_scheme -> proj

1110b7f

Add checks for being standard graded

8839c79

Remove possibly redundant proj function

35b8209

proj(Q::MPolyQuoRing{MPolyDecRingElem{T, PT}}) where {T, PT<:MPolyRingElem{T}} = ProjectiveScheme(Q)

Fix docs

c23e8ca

Fix duplicate exports entry

d41795a

Use script to rename all other functions and types

0247d48

Fix sorting script, sort src/exports.jl

1a227b4

Rename two test files

f313e20

joschmitt reviewed Feb 23, 2024

View reviewed changes

paemurru and others added 3 commits February 23, 2024 23:03

Merge branch 'oscar-system:master' into EP/RenameSpecToAffineScheme

5fd8b4c

Fix symbolic link

1b31ab0

Strange biproduct order in hermitian_miranda_morrison.jl

6b2d4f7

Remove commented question that I made earlier. Better to make an issue or ask someone than comment the code.

lgoettgens added renaming backport 1.0.x Should be backported to the release 1.0 branch labels Feb 23, 2024

lgoettgens reviewed Feb 23, 2024

View reviewed changes

thofma previously requested changes Feb 24, 2024

View reviewed changes

paemurru and others added 2 commits February 25, 2024 17:26

Merge branch 'oscar-system:master' into EP/RenameSpecToAffineScheme

a4f6269

Undo everything in cb13aad except projectivization

8b7dc97

Fix one forgotten proj from last commit

d52731f

thofma requested a review from simonbrandhorst February 25, 2024 20:43

simonbrandhorst reviewed Feb 26, 2024

View reviewed changes

simonbrandhorst added 2 commits February 26, 2024 09:51

Apply suggestions from code review

70c527f

add support for projective_scheme and affine_scheme

501ad23

simonbrandhorst approved these changes Feb 26, 2024

View reviewed changes

simonbrandhorst enabled auto-merge (squash) February 26, 2024 09:13

simonbrandhorst added 2 commits February 26, 2024 10:14

fix typo

532a47e

fix typo

32404e0

simonbrandhorst disabled auto-merge February 26, 2024 09:21

simonbrandhorst enabled auto-merge (squash) February 26, 2024 09:23

lgoettgens mentioned this pull request Feb 26, 2024

Backports Oscar 1.0 #3429

Merged

31 tasks

simonbrandhorst merged commit b00d5e4 into oscar-system:master Feb 26, 2024
20 checks passed

benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 26, 2024

paemurru deleted the EP/RenameSpecToAffineScheme branch February 27, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ep/ Rename Spec to AffineScheme #3345 #3425

Ep/ Rename Spec to AffineScheme #3345 #3425

paemurru commented Feb 23, 2024 •

edited

joschmitt Feb 23, 2024

paemurru Feb 23, 2024

codecov bot commented Feb 23, 2024 •

edited

lgoettgens Feb 23, 2024

paemurru Feb 23, 2024

lgoettgens Feb 23, 2024

thofma left a comment

lgoettgens commented Feb 24, 2024

paemurru commented Feb 24, 2024

paemurru commented Feb 24, 2024

fieker commented Feb 24, 2024

paemurru commented Feb 24, 2024

paemurru commented Feb 24, 2024

thofma commented Feb 24, 2024

paemurru commented Feb 25, 2024

thofma commented Feb 25, 2024

fingolfin commented Feb 26, 2024

simonbrandhorst Feb 26, 2024

simonbrandhorst Feb 26, 2024

simonbrandhorst Feb 26, 2024

lgoettgens Feb 26, 2024

simonbrandhorst Feb 26, 2024

simonbrandhorst left a comment

paemurru commented Feb 26, 2024

fingolfin commented Feb 27, 2024

		is_standard_graded(S) \|\| error("ring must be standard graded")
		return ProjectiveScheme(S)

		# execute: sort < src/exports.jl > dummy ; mv dummy src/exports.jl
		# execute: LC_COLLATE=C sort < src/exports.jl > dummy ; mv dummy src/exports.jl

		@@ -1 +0,0 @@
		../../../test/Serialization/setup_tests.jl

Ep/ Rename Spec to AffineScheme #3345 #3425

Ep/ Rename Spec to AffineScheme #3345 #3425

Conversation

paemurru commented Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 23, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma left a comment

Choose a reason for hiding this comment

lgoettgens commented Feb 24, 2024

paemurru commented Feb 24, 2024

paemurru commented Feb 24, 2024

fieker commented Feb 24, 2024

paemurru commented Feb 24, 2024

paemurru commented Feb 24, 2024

thofma commented Feb 24, 2024

paemurru commented Feb 25, 2024

thofma commented Feb 25, 2024

fingolfin commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrandhorst left a comment

Choose a reason for hiding this comment

paemurru commented Feb 26, 2024

fingolfin commented Feb 27, 2024

paemurru commented Feb 23, 2024 •

edited

codecov bot commented Feb 23, 2024 •

edited