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

Sb/print local rings #2806

Merged
merged 2 commits into from
Sep 17, 2023
Merged

Sb/print local rings #2806

merged 2 commits into from
Sep 17, 2023

Conversation

simonbrandhorst
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Merging #2806 (f549b08) into master (6ed4e08) will decrease coverage by 0.04%.
Report is 1 commits behind head on master.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2806      +/-   ##
==========================================
- Coverage   73.67%   73.64%   -0.04%     
==========================================
  Files         455      455              
  Lines       64527    64569      +42     
==========================================
+ Hits        47541    47552      +11     
- Misses      16986    17017      +31     
Files Changed Coverage
experimental/PlaneCurve/src/PlaneCurve.jl ø
experimental/Schemes/BlowupMorphism.jl ø
experimental/Schemes/CartierDivisor.jl ø
experimental/Schemes/CoherentSheaves.jl ø
experimental/Schemes/FunctionFields.jl ø
experimental/Schemes/IdealSheaves.jl ø
...erimental/Schemes/MorphismFromRationalFunctions.jl ø
experimental/Schemes/Sheaves.jl ø
experimental/Schemes/duValSing.jl ø
...Schemes/AffineAlgebraicSet/Objects/Constructors.jl ø
... and 39 more

Copy link
Collaborator

@afkafkafk13 afkafkafk13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New printing looks very good: right balance between necessary information and sufficiently compact presentation. Thank you.

I left a few comments on particular cases of output, which struck me as odd or weird at first glance or for which I would have expected something else. These are to be understood as markers for further discussion, not as complaints against this PR.

In particular, we should discuss at some point whether the affine space encoded as Spec(k[x]) and the affine space encoded as Spec(k[x]/<0>) or as V() should be printed in the same or different ways. Same question for the empty set and V(1) etc.

experimental/Schemes/BlowupMorphism.jl Show resolved Hide resolved
to [x1, x2, x3] affine 3-space over QQ
given by the pullback function
x1 -> 0
x2 -> x2
x3 -> x3

julia> graph(f)
(Spec of quotient of multivariate polynomial ring, Morphism: spec of quotient of multivariate polynomial ring -> spec of quotient of multivariate polynomial ring, Morphism: spec of quotient of multivariate polynomial ring -> affine 3-space over QQ with coordinates [x1, x2, x3])
(V(x1, -x1, x2 - x2, x3 - x3), Morphism: V(x1, -x1, x2 - x2, x3 - x3) -> V(x1), Morphism: V(x1, -x1, x2 - x2, x3 - x3) -> affine 3-space over QQ with coordinates [x1, x2, x3])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is going wrong with the variable names here. This keeps the new output from being more helpful than the old one.
(I am aware that it is a non-trivial design choice, what to do in the case of products with the same variable names.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could choose new names for the codomain in case of an automorphism. Say x,y,t and x',y',t'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this would be a viable solution for this case, but what about morphisms between rings which only happen to have the same variable names by chance? We should discuss this point directly (not on github).

by ideal(x^2 - y^2 + z^2)

julia> singular_locus(A3)
(Spec of quotient of multivariate polynomial ring, Morphism: spec of quotient of multivariate polynomial ring -> spec of multivariate polynomial ring)
(V(1), Morphism: V(1) -> affine 3-space)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again just a remark on the design decision, certainly not a change request:
empty set would be more user friendlc than V(1) -- but has the same mathematical content. But on the other hand large insatisfiable systems of equations might hide an empty set, whereas computing that it is the empty set may be expensive. One way out might be to check whether the set itself knows that it is the empty set and only then print èmpty set` and print the generating set otherwise.

@simonbrandhorst simonbrandhorst merged commit 3689155 into master Sep 17, 2023
13 of 15 checks passed
@simonbrandhorst simonbrandhorst deleted the sb/print_local_rings branch September 17, 2023 18:14
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.

2 participants